This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: abstract wide int binop code from VRP


On Wed, Jul 11, 2018 at 8:48 AM Aldy Hernandez <aldyh@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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]