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 Thu, Jul 12, 2018 at 10:12 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On 07/11/2018 01:33 PM, Richard Sandiford wrote:
> > Aldy Hernandez <aldyh@redhat.com> writes:
> >> On 07/11/2018 08:52 AM, Richard Biener wrote:
> >>> 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 ;)
> >>
> >> Heh...and ANSI C.
> >>
> >>>
> >>> 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?
> >>
> >> I tried both, but inlining looked cleaner :).  Done.
> >>
> >>>
> >>> 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.
> >>
> >> This aborted:
> >> gcc_assert (NUM_POLY_INT_COEFFS != 1);
> >>
> >> but even taking it out made the bootstrap die somewhere else.
> >>
> >> If it's ok, I'd rather not tackle this now, as I have some more cleanups
> >> that are pending on this.  If you feel strongly, I could do it at a
> >> later time.
> >>
> >> OK pending tests?
> >
> > LGTM FWIW, just some nits:
> >
> >> -/* Subroutine of int_const_binop_1 that handles two INTEGER_CSTs.  */
> >> +/* Combine two wide ints ARG1 and ARG2 under operation CODE to produce
> >> +   a new constant in RES.  Return FALSE if we don't know how to
> >> +   evaluate CODE at compile-time.  */
> >>
> >> -static tree
> >> -int_const_binop_2 (enum tree_code code, const_tree parg1, const_tree parg2,
> >> -               int overflowable)
> >> +bool
> >> +wide_int_binop (enum tree_code code,
> >> +            wide_int &res, const wide_int &arg1, const wide_int &arg2,
> >> +            signop sign, wi::overflow_type &overflow)
> >>   {
> >
> > IMO we should avoid pass-back by reference like the plague. :-)
> > It's especially confusing when the code does things like:
> >
> >      case FLOOR_DIV_EXPR:
> >        if (arg2 == 0)
> >       return false;
> >        res = wi::div_floor (arg1, arg2, sign, &overflow);
> >        break;
>  >
>  > It looked at first like it was taking the address of a local variable
>  > and failing to propagate the information back up.
>  >
>  > I think we should stick to using pointers for this kind of thing.
>  >
>
> Hmmm, I kinda like them.  It just takes some getting used to, but
> generally yields cleaner code as you don't have to keep using '*'
> everywhere.  Plus, the callee can assume the pointer is non-zero.
>
> >> -/* Combine two integer constants PARG1 and PARG2 under operation CODE
> >> -   to produce a new constant.  Return NULL_TREE if we don't know how
> >> +/* Combine two poly int's ARG1 and ARG2 under operation CODE to
> >> +   produce a new constant in RES.  Return FALSE if we don't know how
> >>      to evaluate CODE at compile-time.  */
> >>
> >> -static tree
> >> -int_const_binop_1 (enum tree_code code, const_tree arg1, const_tree arg2,
> >> -               int overflowable)
> >> +static bool
> >> +poly_int_binop (poly_wide_int &res, enum tree_code code,
> >> +            const_tree arg1, const_tree arg2,
> >> +            signop sign, wi::overflow_type &overflow)
> >>   {
> >
> > Would be good to be consistent about the order of the result and code
> > arguments.  Here it's "result, code" (which seems better IMO),
> > but in wide_int_binop it's "code, result".
>
> Agreed.  Fixed.
>
> >
> >> +/* Combine two integer constants PARG1 and PARG2 under operation CODE
> >> +   to produce a new constant.  Return NULL_TREE if we don't know how
> >> +   to evaluate CODE at compile-time.  */
> >> +
> >>   tree
> >> -int_const_binop (enum tree_code code, const_tree arg1, const_tree arg2)
> >> +int_const_binop (enum tree_code code, const_tree arg1, const_tree arg2,
> >> +             int overflowable)
> >
> > s/PARG/ARG/g in comment.
>
> Fixed.
>
> >
> >>   {
> >> -  return int_const_binop_1 (code, arg1, arg2, 1);
> >> +  bool success = false;
> >> +  poly_wide_int poly_res;
> >> +  tree type = TREE_TYPE (arg1);
> >> +  signop sign = TYPE_SIGN (type);
> >> +  wi::overflow_type overflow = wi::OVF_NONE;
> >> +
> >> +  if (TREE_CODE (arg1) == INTEGER_CST && TREE_CODE (arg2) == INTEGER_CST)
> >> +    {
> >> +      wide_int warg1 = wi::to_wide (arg1), res;
> >> +      wide_int warg2 = wi::to_wide (arg2, TYPE_PRECISION (type));
> >> +      success = wide_int_binop (code, res, warg1, warg2, sign, overflow);
> >> +      poly_res = res;
> >> +    }
> >> +  else if (poly_int_tree_p (arg1) && poly_int_tree_p (arg2))
> >> +    success = poly_int_binop (poly_res, code, arg1, arg2, sign, overflow);
> >> +  if (success)
> >> +    return force_fit_type (type, poly_res, overflowable,
> >> +                       (((sign == SIGNED || overflowable == -1)
> >> +                         && overflow)
> >> +                        | TREE_OVERFLOW (arg1) | TREE_OVERFLOW (arg2)));
> >> +  return NULL_TREE;
> >>   }
> >>
> >>   /* Return true if binary operation OP distributes over addition in operand
> >> @@ -1925,7 +1930,7 @@ size_binop_loc (location_t loc, enum tree_code code, tree arg0, tree arg1)
> >>         /* Handle general case of two integer constants.  For sizetype
> >>            constant calculations we always want to know about overflow,
> >>       even in the unsigned case.  */
> >> -      tree res = int_const_binop_1 (code, arg0, arg1, -1);
> >> +      tree res = int_const_binop (code, arg0, arg1, -1);
> >>         if (res != NULL_TREE)
> >>      return res;
> >>       }
> >> diff --git a/gcc/fold-const.h b/gcc/fold-const.h
> >> index 4613a62e1f6..b921ba86854 100644
> >> --- a/gcc/fold-const.h
> >> +++ b/gcc/fold-const.h
> >> @@ -100,7 +100,10 @@ extern tree fold_bit_and_mask (tree, tree, enum tree_code,
> >>                             tree, enum tree_code, tree, tree,
> >>                             tree, enum tree_code, tree, tree, tree *);
> >>   extern tree fold_read_from_constant_string (tree);
> >> -extern tree int_const_binop (enum tree_code, const_tree, const_tree);
> >> +extern bool wide_int_binop (enum tree_code, wide_int &res,
> >> +                        const wide_int &arg1, const wide_int &arg2,
> >> +                        signop, wi::overflow_type &);
> >> +extern tree int_const_binop (enum tree_code, const_tree, const_tree, int = 1);
> >>   #define build_fold_addr_expr(T)\
> >>           build_fold_addr_expr_loc (UNKNOWN_LOCATION, (T))
> >>   extern tree build_fold_addr_expr_loc (location_t, tree);
> >> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> >> index a7453abba7f..7764f7b30b6 100644
> >> --- a/gcc/tree-vrp.c
> >> +++ b/gcc/tree-vrp.c
> >> @@ -956,11 +956,13 @@ value_range_constant_singleton (value_range *vr)
> >>     return NULL_TREE;
> >>   }
> >>
> >> -/* Wrapper around int_const_binop.  Return true if we can compute the
> >> -   result; i.e. if the operation doesn't overflow or if the overflow is
> >> -   undefined.  In the latter case (if the operation overflows and
> >> -   overflow is undefined), then adjust the result to be -INF or +INF
> >> -   depending on CODE, VAL1 and VAL2.  Return the value in *RES.
> >> +/* Wrapper around wide_int_binop that adjusts for overflow.
> >> +
> >> +   Return true if we can compute the result; i.e. if the operation
> >> +   doesn't overflow or if the overflow is undefined.  In the latter
> >> +   case (if the operation overflows and overflow is undefined), then
> >> +   adjust the result to be -INF or +INF depending on CODE, VAL1 and
> >> +   VAL2.  Return the value in *RES.
> >>
> >>      Return false for division by zero, for which the result is
> >>      indeterminate.  */
> >> @@ -970,78 +972,36 @@ vrp_int_const_binop (enum tree_code code, tree val1, tree val2, wide_int *res)
> >>   {
> >>     wi::overflow_type overflow = wi::OVF_NONE;
> >>     signop sign = TYPE_SIGN (TREE_TYPE (val1));
> >> +  wide_int w1 = wi::to_wide (val1);
> >> +  wide_int w2 = wi::to_wide (val2);
> >>
> >
> > This doesn't come for free.  Until we can use "auto", it might be better
> > to keep the wi::to_wide calls at the point they're used.  That probably
> > maans having a separate call to wide_int_binop for shifts.
>
> I would prefer cleaner rather than saving a few microseconds or bytes,
> unless it's in the critical path.  This particular code doesn't seem to
> be ??.

So besides the general discussion about references/pointers for out parameters
let's stay consistet within related APIs.  This means wide_int_binop should
have a

wide_int
wide_int_binop (enum tree_code, const wide_int &, const wide_int &,
signop, wi::overflow_type *)

signature.  Notice how I elided the out wide_int parameter to be the
return value which means
the function isn't supposed to fail which means gcc_unreachable () for
"unhandled" tree codes.
It's more like an exceptional state anyway.

The same goes for the poly_int_binop signature.

The already existing wi::accumulate_overflow should probably be re-done as

wi::overflow_type wi::accumulate_overflow (wi::overflow_type,
wi::overflow_type);

Richard.

> Thanks for the review!
> Aldy


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