On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
>
> Hi Richard,
>
> Thanks for the review.
>
> On 1 June 2018 at 22:20, Richard Biener <richard.guent...@gmail.com> wrote:
> > On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah
> > <kugan.vivekanandara...@linaro.org> wrote:
> >>
> >> Hi Richard,
> >>
> >> This is the revised patch based on the review and the discussion in
> >> https://gcc.gnu.org/ml/gcc/2018-05/msg00179.html.
> >>
> >> In summary:
> >> - I skipped  (element_precision (type) < element_precision (TREE_TYPE
> >> (@0))) in the match.pd pattern as this would prevent transformation
> >> for the case in PR.
> >> that is, I am interested in is something like:
> >>   char t = (char) ABS_EXPR <(int) x>
> >> and I want to generate
> >> char t = (char) ABSU_EXPR <x>
> >>
> >> - I also haven't added all the necessary match.pd changes for
> >> ABSU_EXPR. I have a patch for that but will submit separately based on
> >> this reveiw.
> >>
> >> - I also tried to add ABSU_EXPRsupport  in the places as necessary by
> >> grepping for ABS_EXPR.
> >>
> >> - I also had to add special casing in vectorizer for ABSU_EXP as its
> >> result is unsigned type.
> >>
> >> Is this OK. Patch bootstraps and the regression test is ongoing.
> >
> > The c/c-typeck.c:build_unary_op change looks unnecessary - the
> > C FE should never generate this directly (the c-common one might
> > be triggered by early folding I guess).
>
> The Gimple FE testcase is running into this.

Ah, OK then.

> >
> > @@ -1761,6 +1762,9 @@ const_unop (enum tree_code code, tree type, tree arg0)
> >        if (TREE_CODE (arg0) == INTEGER_CST || TREE_CODE (arg0) == REAL_CST)
> >         return fold_abs_const (arg0, type);
> >        break;
> > +    case ABSU_EXPR:
> > +       return fold_convert (type, fold_abs_const (arg0,
> > +                                                  signed_type_for (type)));
> >
> >      case CONJ_EXPR:
> >
> > I think this will get you bogus TREE_OVERFLOW flags set on ABSU (-INT_MIN).
> >
> > I think you want to change fold_abs_const to properly deal with arg0 being
> > signed and type unsigned.  That is, sth like
> >
> > diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> > index 6f80f1b1d69..f60f9c77e91 100644
> > --- a/gcc/fold-const.c
> > +++ b/gcc/fold-const.c
> > @@ -13843,18 +13843,19 @@ fold_abs_const (tree arg0, tree type)
> >        {
> >          /* If the value is unsigned or non-negative, then the absolute 
> > value
> >            is the same as the ordinary value.  */
> > -       if (!wi::neg_p (wi::to_wide (arg0), TYPE_SIGN (type)))
> > -         t = arg0;
> > +       wide_int val = wi::to_wide (arg0);
> > +       bool overflow = false;
> > +       if (!wi::neg_p (val, TYPE_SIGN (TREE_TYPE (arg0))))
> > +         ;
> >
> >         /* If the value is negative, then the absolute value is
> >            its negation.  */
> >         else
> > -         {
> > -           bool overflow;
> > -           wide_int val = wi::neg (wi::to_wide (arg0), &overflow);
> > -           t = force_fit_type (type, val, -1,
> > -                               overflow | TREE_OVERFLOW (arg0));
> > -         }
> > +         wide_int val = wi::neg (val, &overflow);
> > +
> > +       /* Force to the destination type, set TREE_OVERFLOW for signed
> > +          TYPE only.  */
> > +       t = force_fit_type (type, val, 1, overflow | TREE_OVERFLOW (arg0));
> >        }
> >        break;
> >
> > and then simply share the const_unop code with ABS_EXPR.
>
> Done.
>
> > diff --git a/gcc/match.pd b/gcc/match.pd
> > index 14386da..7d7c132 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -102,6 +102,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >  (match (nop_convert @0)
> >   @0)
> >
> > +(simplify (abs (convert @0))
> > + (if (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> > +      && !TYPE_UNSIGNED (TREE_TYPE (@0))
> > +      && !TYPE_UNSIGNED (type))
> > +  (with { tree utype = unsigned_type_for (TREE_TYPE (@0)); }
> > +   (convert (absu:utype @0)))))
> > +
> > +
> >
> > please put a comment before the pattern.  I believe there's no
> > need to check for !TYPE_UNSIGNED (type).  Note this
> > also converts abs ((char)int-var) to (char)absu(int-var) which
> > doesn't make sense.  The original issue you want to address
> > here is the case where TYPE_PRECISION of @0 is less than
> > the precision of type.  That is, you want to remove language
> > introduced integer promotion of @0 which only is possible
> > with ABSU.  So please do add such precision check
> > (I simply suggested the bogus direction of the test).
>
> Done.
> >
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index 68f4fd3..9b62583 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -3685,6 +3685,12 @@ verify_gimple_assign_unary (gassign *stmt)
> >      case PAREN_EXPR:
> >      case CONJ_EXPR:
> >        break;
> > +    case ABSU_EXPR:
> > +      if (!TYPE_UNSIGNED (lhs_type)
> > +         || !ANY_INTEGRAL_TYPE_P (rhs1_type))
> >
> >  if (!ANY_INTEGRAL_TYPE_P (lhs_type)
> >      || !TYPE_UNSIGNED (lhs_type)
> >      || !ANY_INTEGRAL_TYPE_P (rhs1_type)
> >      || TYPE_UNSIGNED (rhs1_type)
> >      || element_precision (lhs_type) != element_precision (rhs1_type))
> >   {
> >       error ("invalid types for ABSU_EXPR");
> >       debug_generic_expr (lhs_type);
> >       debug_generic_expr (rhs1_type);
> >      return true;
> >   }
> >

^^^  you forgot this one.


> > +       return true;
> > +      return false;
> > +      break;
> >
> > diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
> > index 30c6d9e..44b1399 100644
> > --- a/gcc/tree-eh.c
> > +++ b/gcc/tree-eh.c
> > @@ -2465,6 +2465,7 @@ operation_could_trap_helper_p (enum tree_code op,
> >
> >      case NEGATE_EXPR:
> >      case ABS_EXPR:
> > +    case ABSU_EXPR:
> >      case CONJ_EXPR:
> >        /* These operations don't trap with floating point.  */
> >        if (honor_trapv)
> >
> > ABSU never traps.  Please instead unconditionally return false.
> Done.
>
> >
> > diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> > index 66c78de..b52d714 100644
> > --- a/gcc/tree-vect-stmts.c
> > +++ b/gcc/tree-vect-stmts.c
> > @@ -5995,7 +5995,11 @@ vectorizable_operation (gimple *stmt,
> > gimple_stmt_iterator *gsi,
> >                       "transform binary/unary operation.\n");
> >
> >    /* Handle def.  */
> > -  vec_dest = vect_create_destination_var (scalar_dest, vectype);
> > +  if (code == ABSU_EXPR)
> > +    vec_dest = vect_create_destination_var (scalar_dest,
> > +                                           unsigned_type_for (vectype));
> > +  else
> > +    vec_dest = vect_create_destination_var (scalar_dest, vectype);
> >
> >    /* POINTER_DIFF_EXPR has pointer arguments which are vectorized as
> >       vectors with unsigned elements, but the result is signed.  So, we
> >
> > simply use vectype_out for creation of vec_dest.
> Done.

   /* Handle def.  */
-  vec_dest = vect_create_destination_var (scalar_dest, vectype);
+  if (code == ABSU_EXPR)
+    vec_dest = vect_create_destination_var (scalar_dest, vectype_out);
+  else
+    vec_dest = vect_create_destination_var (scalar_dest, vectype);

I meant _always_ vectype_out.  Thus unconditionally

  vec_dest = vect_create_destination_var (scalar_dest, vectype_out);

OK with those two changes.

Thanks,
Richard.

> >
> > diff --git a/gcc/tree.def b/gcc/tree.def
> > index c660b2c..5fec781 100644
> > --- a/gcc/tree.def
> > +++ b/gcc/tree.def
> > @@ -760,6 +760,7 @@ DEFTREECODE (MAX_EXPR, "max_expr", tcc_binary, 2)
> >     An ABS_EXPR must have either an INTEGER_TYPE or a REAL_TYPE.  The
> >     operand of the ABS_EXPR must have the same type.  */
> >  DEFTREECODE (ABS_EXPR, "abs_expr", tcc_unary, 1)
> > +DEFTREECODE (ABSU_EXPR, "absu_expr", tcc_unary, 1)
> >
> >  /* Shift operations for shift and rotate.
> >     Shift means logical shift if done on an
> >
> > You can clearly see that the comment before ABS_EXPR doesn't apply to 
> > ABSU_EXPR
> > so please add an appropriate one.  I suggest
> >
> > /* Represents the unsigned absolute value of the operand.
> >     An ABSU_EXPR must have unsigned INTEGER_TYPE.  The operand of the 
> > ABSU_EXPR
> >     must have the corresponding signed type.  */
>
> Done.
>
> Here is the reviesed patch. Is this OK?
>
> Thanks,
> Kugan
>
> >
> > Otherwise looks OK.  (I didn't explicitely check for missing ABSU_EXPR
> > handling this time)
> >
> > Thanks,
> > Richard.
> >
> >
> >> Thanks,
> >> Kugan
> >>
> >>
> >> On 18 May 2018 at 12:36, Kugan Vivekanandarajah
> >> <kugan.vivekanandara...@linaro.org> wrote:
> >> > Hi Richard,
> >> >
> >> > Thanks for the review. I am revising the patch based on Andrew's 
> >> > comments too.
> >> >
> >> > On 17 May 2018 at 20:36, Richard Biener <richard.guent...@gmail.com> 
> >> > wrote:
> >> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pins...@gmail.com> wrote:
> >> >>
> >> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah
> >> >>> <kugan.vivekanandara...@linaro.org> wrote:
> >> >>> > As mentioned in the PR, I am trying to add ABSU_EXPR to fix this
> >> >>> > issue. In the attached patch, in fold_cond_expr_with_comparison I am
> >> >>> > generating ABSU_EXPR for these cases. As I understand, absu_expr is
> >> >>> > well defined in RTL. So, the issue is generating absu_expr  and
> >> >>> > transferring to RTL in the correct way. I am not sure I am not doing
> >> >>> > all that is needed. I will clean up and add more test-cases based on
> >> >>> > the feedback.
> >> >>
> >> >>
> >> >>> diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c
> >> >>> index 71e172c..2b812e5 100644
> >> >>> --- a/gcc/optabs-tree.c
> >> >>> +++ b/gcc/optabs-tree.c
> >> >>> @@ -235,6 +235,7 @@ optab_for_tree_code (enum tree_code code, 
> >> >>> const_tree
> >> >> type,
> >> >>>         return trapv ? negv_optab : neg_optab;
> >> >>
> >> >>>       case ABS_EXPR:
> >> >>> +    case ABSU_EXPR:
> >> >>>         return trapv ? absv_optab : abs_optab;
> >> >>
> >> >>
> >> >>> This part is not correct, it should something like this:
> >> >>
> >> >>>       case ABS_EXPR:
> >> >>>         return trapv ? absv_optab : abs_optab;
> >> >>> +    case ABSU_EXPR:
> >> >>> +       return abs_optab ;
> >> >>
> >> >>> Because ABSU is not undefined at the TYPE_MAX.
> >> >>
> >> >> Also
> >> >>
> >> >>         /* Unsigned abs is simply the operand.  Testing here means we 
> >> >> don't
> >> >>           risk generating incorrect code below.  */
> >> >> -      if (TYPE_UNSIGNED (type))
> >> >> +      if (TYPE_UNSIGNED (type)
> >> >> +         && (code != ABSU_EXPR))
> >> >>          return op0;
> >> >>
> >> >> is wrong.  ABSU of an unsigned number is still just that number.
> >> >>
> >> >> The change to fold_cond_expr_with_comparison looks odd to me
> >> >> (premature optimization).  It should be done separately - it seems
> >> >> you are doing
> >> >
> >> > FE seems to be using this to generate ABS_EXPR from
> >> > c_fully_fold_internal to fold_build3_loc and so on. I changed this to
> >> > generate ABSU_EXPR for the case in the testcase. So the question
> >> > should be, in what cases do we need ABS_EXPR and in what cases do we
> >> > need ABSU_EXPR. It is not very clear to me.
> >> >
> >> >
> >> >>
> >> >> (simplify (abs (convert @0)) (convert (absu @0)))
> >> >>
> >> >> here.
> >> >>
> >> >> You touch one other place in fold-const.c but there seem to be many
> >> >> more that need ABSU_EXPR handling (you touched the one needed
> >> >> for correctness) - esp. you should at least handle constant folding
> >> >> in const_unop and the nonnegative predicate.
> >> >
> >> > OK.
> >> >>
> >> >> @@ -3167,6 +3167,9 @@ verify_expr (tree *tp, int *walk_subtrees, void 
> >> >> *data
> >> >> ATTRIBUTE_UNUSED)
> >> >>         CHECK_OP (0, "invalid operand to unary operator");
> >> >>         break;
> >> >>
> >> >> +    case ABSU_EXPR:
> >> >> +      break;
> >> >> +
> >> >>       case REALPART_EXPR:
> >> >>       case IMAGPART_EXPR:
> >> >>
> >> >> verify_expr is no more.  Did you test this recently against trunk?
> >> >
> >> > This patch is against slightly older trunk. I will rebase it.
> >> >
> >> >>
> >> >> @@ -3937,6 +3940,9 @@ verify_gimple_assign_unary (gassign *stmt)
> >> >>       case PAREN_EXPR:
> >> >>       case CONJ_EXPR:
> >> >>         break;
> >> >> +    case ABSU_EXPR:
> >> >> +      /* FIXME.  */
> >> >> +      return false;
> >> >>
> >> >> no - please not!  Please add verification here - ABSU should be only
> >> >> called on INTEGRAL, vector or complex INTEGRAL types and the
> >> >> type of the LHS should be always the unsigned variant of the
> >> >> argument type.
> >> >
> >> > OK.
> >> >>
> >> >>     if (is_gimple_val (cond_expr))
> >> >>       return cond_expr;
> >> >>
> >> >> -  if (TREE_CODE (cond_expr) == ABS_EXPR)
> >> >> +  if (TREE_CODE (cond_expr) == ABS_EXPR
> >> >> +      || TREE_CODE (cond_expr) == ABSU_EXPR)
> >> >>       {
> >> >>         rhs1 = TREE_OPERAND (cond_expr, 1);
> >> >>         STRIP_USELESS_TYPE_CONVERSION (rhs1);
> >> >>
> >> >> err, but the next line just builds a ABS_EXPR ...
> >> >>
> >> >> How did you identify spots that need adjustment?  I would expect that
> >> >> once folding generates ABSU_EXPR that you need to adjust frontends
> >> >> (C++ constexpr handling for example).  Also I miss adjustments
> >> >> to gimple-pretty-print.c and the GIMPLE FE parser.
> >> >
> >> > I will add this.
> >> >>
> >> >> recursively grepping throughout the whole gcc/ tree doesn't reveal too 
> >> >> many
> >> >> cases of ABS_EXPR so I think it's reasonable to audit all of them.
> >> >>
> >> >> I also miss some trivial absu simplifications in match.pd.  There are 
> >> >> not
> >> >> a lot of abs cases but similar ones would be good to have initially.
> >> >
> >> > I will add them in the next version.
> >> >
> >> > Thanks,
> >> > Kugan
> >> >
> >> >>
> >> >> Thanks for tackling this!
> >> >> Richard.
> >> >>
> >> >>> Thanks,
> >> >>> Andrew
> >> >>
> >> >>> >
> >> >>> > Thanks,
> >> >>> > Kugan
> >> >>> >
> >> >>> >
> >> >>> > gcc/ChangeLog:
> >> >>> >
> >> >>> > 2018-05-13  Kugan Vivekanandarajah  
> >> >>> > <kugan.vivekanandara...@linaro.org>
> >> >>> >
> >> >>> >     * expr.c (expand_expr_real_2): Handle ABSU_EXPR.
> >> >>> >     * fold-const.c (fold_cond_expr_with_comparison): Generate 
> >> >>> > ABSU_EXPR
> >> >>> >     (fold_unary_loc): Handle ABSU_EXPR.
> >> >>> >     * optabs-tree.c (optab_for_tree_code): Likewise.
> >> >>> >     * tree-cfg.c (verify_expr): Likewise.
> >> >>> >     (verify_gimple_assign_unary):  Likewise.
> >> >>> >     * tree-if-conv.c (fold_build_cond_expr):  Likewise.
> >> >>> >     * tree-inline.c (estimate_operator_cost):  Likewise.
> >> >>> >     * tree-pretty-print.c (dump_generic_node):  Likewise.
> >> >>> >     * tree.def (ABSU_EXPR): New.
> >> >>> >
> >> >>> > gcc/testsuite/ChangeLog:
> >> >>> >
> >> >>> > 2018-05-13  Kugan Vivekanandarajah  
> >> >>> > <kugan.vivekanandara...@linaro.org>
> >> >>> >
> >> >>> >     * gcc.dg/absu.c: New test.

Reply via email to