On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez <al...@redhat.com> wrote:
>
> Hmmm, I think we can do better, and since this hasn't been reviewed yet,
> I don't think anyone will mind the adjustment to the patch ;-).
>
> I really hate int_const_binop_SOME_RANDOM_NUMBER. We should abstract
> them into properly named poly_int_binop, wide_int_binop, and tree_binop,
> and then use a default argument for int_const_binop() to get things going.
>
> Sorry for more changes in flight, but I thought we could benefit from
> more cleanups :).
>
> OK for trunk pending tests?
Much of GCC pre-dates function overloading / default args ;)
Looks OK but can you please rename your tree_binop to int_cst_binop?
Or maybe inline it into int_const_binop, also sharing the force_fit_type ()
tail with poly_int_binop?
What about mixed INTEGER_CST / poly_int constants? Shouldn't it
be
if (neither-poly-nor-integer-cst (arg1 || arg2))
return NULL_TREE;
if (poly_int_tree (arg1) || poly_int_tree (arg2))
poly-int-stuff
else if (INTEGER_CST && INTEGER_CST)
wide-int-stuff
? I see that is a pre-existing issue but if you are at refactoring...
wi::to_poly_wide should handle INTEGER_CST operands just fine
I hope.
Thanks,
Richard.
> Aldy
>
> On 07/10/2018 04:31 AM, Aldy Hernandez wrote:
> > Howdy!
> >
> > Attached are more cleanups to VRP getting rid of some repetitive code,
> > as well as abstracting wide int handling code into their own functions.
> > There should be no change to existing functionality.
> >
> > You may notice that I have removed the PLUS/MINUS_EXPR handling in
> > vrp_int_const_binop, even from the new abstracted code:
> >
> > - /* For addition, the operands must be of the same sign
> > - to yield an overflow. Its sign is therefore that
> > - of one of the operands, for example the first. */
> > - || (code == PLUS_EXPR && sgn1 >= 0)
> > - /* For subtraction, operands must be of
> > - different signs to yield an overflow. Its sign is
> > - therefore that of the first operand or the opposite of
> > - that of the second operand. A first operand of 0 counts
> > - as positive here, for the corner case 0 - (-INF), which
> > - overflows, but must yield +INF. */
> > - || (code == MINUS_EXPR && sgn1 >= 0)
> >
> > This code is actually unreachable, as the switch above this snippet was
> > already aborting if code was not one of the shift or mult/div operators.
> >
> > Oh yeah, don't blame me for the cryptic comment to
> > range_easy_mask_min_mask(). That machine language comment was already
> > there ;-).
> >
> > OK pending one more round of tests?
> >
> > Aldy
