[RFC,PR64946] "abs" vectorization fails for char/short types

Message ID CAELXzTPFP5QkdDRscDGTaEPwTLe_ZzrGkuSimhm+DZDfK6dw_w@mail.gmail.com
State New
Headers show
Series
  • [RFC,PR64946] "abs" vectorization fails for char/short types
Related show

Commit Message

Kugan Vivekanandarajah May 17, 2018, 2:14 a.m.
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.

Thanks,
Kugan


gcc/ChangeLog:

2018-05-13  Kugan Vivekanandarajah  <kugan.vivekanandarajah@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.vivekanandarajah@linaro.org>

    * gcc.dg/absu.c: New test.

Comments

Andrew Pinski May 17, 2018, 2:56 a.m. | #1
On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@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.

Thanks,
Andrew

>

> Thanks,

> Kugan

>

>

> gcc/ChangeLog:

>

> 2018-05-13  Kugan Vivekanandarajah  <kugan.vivekanandarajah@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.vivekanandarajah@linaro.org>

>

>     * gcc.dg/absu.c: New test.
Richard Biener May 17, 2018, 10:36 a.m. | #2
On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

> <kugan.vivekanandarajah@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

(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.

@@ -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?

@@ -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.

    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.

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.

Thanks for tackling this!
Richard.

> Thanks,

> Andrew


> >

> > Thanks,

> > Kugan

> >

> >

> > gcc/ChangeLog:

> >

> > 2018-05-13  Kugan Vivekanandarajah  <kugan.vivekanandarajah@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.vivekanandarajah@linaro.org>

> >

> >     * gcc.dg/absu.c: New test.
Kugan Vivekanandarajah May 18, 2018, 2:36 a.m. | #3
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.guenther@gmail.com> wrote:
> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

>

>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

>> >

>> >     * gcc.dg/absu.c: New test.
Richard Biener May 18, 2018, 7:58 a.m. | #4
On Fri, May 18, 2018 at 4:37 AM Kugan Vivekanandarajah <
kugan.vivekanandarajah@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.guenther@gmail.com>

wrote:
> > On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

> >

> >> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

> >> <kugan.vivekanandarajah@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.


Well, all existing ABS_EXPR generations are obviously fine.  Then there
are transforms that are not possible w/o ABSU_EXPR like the narrowing
you performed here.  This is becasue for ABS_EXPR you can't simply avoid
the undefined
behavior by performing the operation on unsigned integers... (that's similar
to signed integer division of -INT_MIN / -1 -- the result is representable
in
unsigned but not in signed).


> >

> > (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.vivekanandarajah@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.vivekanandarajah@linaro.org>
> >> >

> >> >     * gcc.dg/absu.c: New test.
Kugan Vivekanandarajah June 1, 2018, 2:11 a.m. | #5
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.

Thanks,
Kugan


On 18 May 2018 at 12:36, Kugan Vivekanandarajah
<kugan.vivekanandarajah@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.guenther@gmail.com> wrote:

>> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

>>

>>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

>>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

>>> >

>>> >     * gcc.dg/absu.c: New test.
From 6675f0fe71b1de2c2def5c5d2bd150ae1e707b5c Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Date: Tue, 29 May 2018 10:26:23 +1000
Subject: [PATCH] absu v2

Change-Id: Id34948b4d585fbcdcb8ff6eb3728fd3cc6c41bfa
---
 gcc/c-family/c-common.c            |  1 +
 gcc/c/c-typeck.c                   | 10 ++++++++++
 gcc/c/gimple-parser.c              |  9 ++++++++-
 gcc/cfgexpand.c                    |  1 +
 gcc/config/i386/i386.c             |  1 +
 gcc/cp/constexpr.c                 |  1 +
 gcc/cp/cp-gimplify.c               |  1 +
 gcc/dojump.c                       |  1 +
 gcc/expr.c                         |  3 ++-
 gcc/fold-const.c                   |  6 +++++-
 gcc/gimple-pretty-print.c          |  7 +++++--
 gcc/gimple-ssa-backprop.c          |  2 ++
 gcc/match.pd                       |  8 ++++++++
 gcc/optabs-tree.c                  |  2 ++
 gcc/testsuite/gcc.dg/absu.c        | 41 ++++++++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/gimplefe-29.c | 11 ++++++++++
 gcc/tree-cfg.c                     |  6 ++++++
 gcc/tree-eh.c                      |  1 +
 gcc/tree-inline.c                  |  1 +
 gcc/tree-pretty-print.c            |  6 ++++++
 gcc/tree-vect-patterns.c           |  3 ++-
 gcc/tree-vect-stmts.c              |  6 +++++-
 gcc/tree.def                       |  1 +
 23 files changed, 122 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/absu.c
 create mode 100644 gcc/testsuite/gcc.dg/gimplefe-29.c

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 859eeb4..0e8efb5 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3312,6 +3312,7 @@ c_common_truthvalue_conversion (location_t location, tree expr)
 
     case NEGATE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case FLOAT_EXPR:
     case EXCESS_PRECISION_EXPR:
       /* These don't change whether an object is nonzero or zero.  */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 45a4529..5bb6804 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -4314,6 +4314,16 @@ build_unary_op (location_t location, enum tree_code code, tree xarg,
 	arg = default_conversion (arg);
       break;
 
+    case ABSU_EXPR:
+      if (!(typecode == INTEGER_TYPE))
+	{
+	  error_at (location, "wrong type argument to absu");
+	  return error_mark_node;
+	}
+      else if (!noconvert)
+	arg = default_conversion (arg);
+      break;
+
     case CONJ_EXPR:
       /* Conjugating a real value is a no-op, but allow it anyway.  */
       if (!(typecode == INTEGER_TYPE || typecode == REAL_TYPE
diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index 8f1c442..1be5d14 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -328,7 +328,8 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq)
     case CPP_NAME:
       {
 	tree id = c_parser_peek_token (parser)->value;
-	if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0)
+	if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0
+	    || strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0)
 	  goto build_unary_expr;
 	break;
       }
@@ -638,6 +639,12 @@ c_parser_gimple_unary_expression (c_parser *parser)
 	      op = c_parser_gimple_postfix_expression (parser);
 	      return parser_build_unary_op (op_loc, ABS_EXPR, op);
 	    }
+	  else if (strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0)
+	    {
+	      c_parser_consume_token (parser);
+	      op = c_parser_gimple_postfix_expression (parser);
+	      return parser_build_unary_op (op_loc, ABSU_EXPR, op);
+	    }
 	  else
 	    return c_parser_gimple_postfix_expression (parser);
 	}
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 5c323be..d06224d 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -4620,6 +4620,7 @@ expand_debug_expr (tree exp)
       }
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       return simplify_gen_unary (ABS, mode, op0, mode);
 
     case NEGATE_EXPR:
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 637c105..f5a4856 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -51232,6 +51232,7 @@ ix86_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind,
 
 	case BIT_IOR_EXPR:
 	case ABS_EXPR:
+	case ABSU_EXPR:
 	case MIN_EXPR:
 	case MAX_EXPR:
 	case BIT_XOR_EXPR:
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index a099408..30ea091 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -5759,6 +5759,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
     case FLOAT_EXPR:
     case NEGATE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case TRUTH_NOT_EXPR:
     case FIXED_CONVERT_EXPR:
     case UNARY_PLUS_EXPR:
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index b4e23e2..4567365 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2272,6 +2272,7 @@ cp_fold (tree x)
     case FLOAT_EXPR:
     case NEGATE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case BIT_NOT_EXPR:
     case TRUTH_NOT_EXPR:
     case FIXED_CONVERT_EXPR:
diff --git a/gcc/dojump.c b/gcc/dojump.c
index 9da8a0e..88cc96a 100644
--- a/gcc/dojump.c
+++ b/gcc/dojump.c
@@ -467,6 +467,7 @@ do_jump (tree exp, rtx_code_label *if_false_label,
       /* FALLTHRU */
     case NON_LVALUE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case NEGATE_EXPR:
     case LROTATE_EXPR:
     case RROTATE_EXPR:
diff --git a/gcc/expr.c b/gcc/expr.c
index ecc5292..a92c23f 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -9013,6 +9013,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
       return REDUCE_BIT_FIELD (temp);
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       op0 = expand_expr (treeop0, subtarget,
 			 VOIDmode, EXPAND_NORMAL);
       if (modifier == EXPAND_STACK_PARM)
@@ -9024,7 +9025,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
 
       /* Unsigned abs is simply the operand.  Testing here means we don't
 	 risk generating incorrect code below.  */
-      if (TYPE_UNSIGNED (type))
+      if (TYPE_UNSIGNED (TREE_TYPE (treeop0)))
 	return op0;
 
       return expand_abs (mode, op0, target, unsignedp,
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 0f57f07..20b4271 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -1723,7 +1723,8 @@ const_unop (enum tree_code code, tree type, tree arg0)
       && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
       && REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg0))
       && code != NEGATE_EXPR
-      && code != ABS_EXPR)
+      && code != ABS_EXPR
+      && code != ABSU_EXPR)
     return NULL_TREE;
 
   switch (code)
@@ -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:
       if (TREE_CODE (arg0) == COMPLEX_CST)
diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
index 49e9e12..bb4d259 100644
--- a/gcc/gimple-pretty-print.c
+++ b/gcc/gimple-pretty-print.c
@@ -358,14 +358,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc,
       break;
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       if (flags & TDF_GIMPLE)
 	{
-	  pp_string (buffer, "__ABS ");
+	  pp_string (buffer,
+		     rhs_code == ABS_EXPR ? "__ABS " : "__ABSU ");
 	  dump_generic_node (buffer, rhs, spc, flags, false);
 	}
       else
 	{
-	  pp_string (buffer, "ABS_EXPR <");
+	  pp_string (buffer,
+		     rhs_code == ABS_EXPR ? "ABS_EXPR <" : "ABSU_EXPR <");
 	  dump_generic_node (buffer, rhs, spc, flags, false);
 	  pp_greater (buffer);
 	}
diff --git a/gcc/gimple-ssa-backprop.c b/gcc/gimple-ssa-backprop.c
index 9ab655c..bb378a9 100644
--- a/gcc/gimple-ssa-backprop.c
+++ b/gcc/gimple-ssa-backprop.c
@@ -408,6 +408,7 @@ backprop::process_assign_use (gassign *assign, tree rhs, usage_info *info)
   switch (gimple_assign_rhs_code (assign))
     {
     case ABS_EXPR:
+    case ABSU_EXPR:
       /* The sign of the input doesn't matter.  */
       info->flags.ignore_sign = true;
       break;
@@ -675,6 +676,7 @@ strip_sign_op_1 (tree rhs)
     switch (gimple_assign_rhs_code (assign))
       {
       case ABS_EXPR:
+      case ABSU_EXPR:
       case NEGATE_EXPR:
 	return gimple_assign_rhs1 (assign);
 
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)))))
+
+
 /* Simplifications of operations with one constant operand and
    simplifications to constants or single values.  */
 
diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c
index 73e6654..092663b 100644
--- a/gcc/optabs-tree.c
+++ b/gcc/optabs-tree.c
@@ -234,6 +234,8 @@ optab_for_tree_code (enum tree_code code, const_tree type,
     case ABS_EXPR:
       return trapv ? absv_optab : abs_optab;
 
+    case ABSU_EXPR:
+      return abs_optab;
     default:
       return unknown_optab;
     }
diff --git a/gcc/testsuite/gcc.dg/absu.c b/gcc/testsuite/gcc.dg/absu.c
new file mode 100644
index 0000000..81e37a4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/absu.c
@@ -0,0 +1,41 @@
+
+/* { dg-do run  } */
+/* { dg-options "-O0" } */
+
+#include <limits.h>
+#define ABS(x)	(((x) >= 0) ? (x) : -(x))
+
+#define DEF_TEST(TYPE)	\
+void foo_##TYPE (signed TYPE x, unsigned TYPE y){	\
+    TYPE t = ABS (x);				\
+    if (t != y)					\
+ 	__builtin_abort ();			\
+}						\
+
+DEF_TEST (char);
+DEF_TEST (short);
+DEF_TEST (int);
+DEF_TEST (long);
+void main ()
+{
+  foo_char (SCHAR_MIN + 1, SCHAR_MAX);
+  foo_char (0, 0);
+  foo_char (-1, 1);
+  foo_char (1, 1);
+  foo_char (SCHAR_MAX, SCHAR_MAX);
+
+  foo_int (-1, 1);
+  foo_int (0, 0);
+  foo_int (INT_MAX, INT_MAX);
+  foo_int (INT_MIN + 1, INT_MAX);
+
+  foo_short (-1, 1);
+  foo_short (0, 0);
+  foo_short (SHRT_MAX, SHRT_MAX);
+  foo_short (SHRT_MIN + 1, SHRT_MAX);
+
+  foo_long (-1, 1);
+  foo_long (0, 0);
+  foo_long (LONG_MAX, LONG_MAX);
+  foo_long (LONG_MIN + 1, LONG_MAX);
+}
diff --git a/gcc/testsuite/gcc.dg/gimplefe-29.c b/gcc/testsuite/gcc.dg/gimplefe-29.c
new file mode 100644
index 0000000..54b86ef
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gimplefe-29.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fgimple -fdump-tree-ssa-gimple" } */
+
+unsigned int __GIMPLE() f(int a)
+{
+  unsigned int t0;
+  t0_1 = __ABSU a;
+  return t0_1;
+}
+
+/* { dg-final { scan-tree-dump "__ABSU a" "ssa" } } */
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))
+	return true;
+      return false;
+      break;
 
     case VEC_DUPLICATE_EXPR:
       if (TREE_CODE (lhs_type) != VECTOR_TYPE
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)
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 7881131..665e53c 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3865,6 +3865,7 @@ estimate_operator_cost (enum tree_code code, eni_weights *weights,
     case MIN_EXPR:
     case MAX_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
 
     case LSHIFT_EXPR:
     case RSHIFT_EXPR:
diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
index 5a8c8eb..a950a9e 100644
--- a/gcc/tree-pretty-print.c
+++ b/gcc/tree-pretty-print.c
@@ -2463,6 +2463,12 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags,
       pp_greater (pp);
       break;
 
+    case ABSU_EXPR:
+      pp_string (pp, "ABSU_EXPR <");
+      dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false);
+      pp_greater (pp);
+      break;
+
     case RANGE_EXPR:
       NIY;
       break;
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 6da784c..af6b9bd 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -615,7 +615,8 @@ vect_recog_sad_pattern (vec<gimple *> *stmts, tree *type_in,
   gcc_assert (abs_stmt_vinfo);
   if (STMT_VINFO_DEF_TYPE (abs_stmt_vinfo) != vect_internal_def)
     return NULL;
-  if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR)
+  if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR
+      && gimple_assign_rhs_code (abs_stmt) != ABSU_EXPR)
     return NULL;
 
   tree abs_oprnd = gimple_assign_rhs1 (abs_stmt);
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
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
Richard Biener June 1, 2018, 12:20 p.m. | #6
On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@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).

@@ -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.

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).

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;
  }

+       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.

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.

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.  */

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.vivekanandarajah@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.guenther@gmail.com> wrote:

> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

> >>

> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

> >>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

> >>> >

> >>> >     * gcc.dg/absu.c: New test.
Kugan Vivekanandarajah June 4, 2018, 8:18 a.m. | #7
Hi Richard,

Thanks for the review.

On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote:
> On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah

> <kugan.vivekanandarajah@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.

>

> @@ -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;

>   }

>

> +       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.

>

> 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.vivekanandarajah@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.guenther@gmail.com> wrote:

>> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

>> >>

>> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

>> >>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

>> >>> >

>> >>> >     * gcc.dg/absu.c: New test.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 859eeb4..0e8efb5 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3312,6 +3312,7 @@ c_common_truthvalue_conversion (location_t location, tree expr)
 
     case NEGATE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case FLOAT_EXPR:
     case EXCESS_PRECISION_EXPR:
       /* These don't change whether an object is nonzero or zero.  */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 45a4529..5bb6804 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -4314,6 +4314,16 @@ build_unary_op (location_t location, enum tree_code code, tree xarg,
 	arg = default_conversion (arg);
       break;
 
+    case ABSU_EXPR:
+      if (!(typecode == INTEGER_TYPE))
+	{
+	  error_at (location, "wrong type argument to absu");
+	  return error_mark_node;
+	}
+      else if (!noconvert)
+	arg = default_conversion (arg);
+      break;
+
     case CONJ_EXPR:
       /* Conjugating a real value is a no-op, but allow it anyway.  */
       if (!(typecode == INTEGER_TYPE || typecode == REAL_TYPE
diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index c9abe24..11e76ff 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -328,7 +328,8 @@ c_parser_gimple_statement (c_parser *parser, gimple_seq *seq)
     case CPP_NAME:
       {
 	tree id = c_parser_peek_token (parser)->value;
-	if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0)
+	if (strcmp (IDENTIFIER_POINTER (id), "__ABS") == 0
+	    || strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0)
 	  goto build_unary_expr;
 	break;
       }
@@ -638,6 +639,12 @@ c_parser_gimple_unary_expression (c_parser *parser)
 	      op = c_parser_gimple_postfix_expression (parser);
 	      return parser_build_unary_op (op_loc, ABS_EXPR, op);
 	    }
+	  else if (strcmp (IDENTIFIER_POINTER (id), "__ABSU") == 0)
+	    {
+	      c_parser_consume_token (parser);
+	      op = c_parser_gimple_postfix_expression (parser);
+	      return parser_build_unary_op (op_loc, ABSU_EXPR, op);
+	    }
 	  else
 	    return c_parser_gimple_postfix_expression (parser);
 	}
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index ef143a3..ba4543c 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -4621,6 +4621,7 @@ expand_debug_expr (tree exp)
       }
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       return simplify_gen_unary (ABS, mode, op0, mode);
 
     case NEGATE_EXPR:
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index e19864d..487127c 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -51228,6 +51228,7 @@ ix86_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind,
 
 	case BIT_IOR_EXPR:
 	case ABS_EXPR:
+	case ABSU_EXPR:
 	case MIN_EXPR:
 	case MAX_EXPR:
 	case BIT_XOR_EXPR:
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 8c6ec55..bea9147 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -5760,6 +5760,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
     case FLOAT_EXPR:
     case NEGATE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case TRUTH_NOT_EXPR:
     case FIXED_CONVERT_EXPR:
     case UNARY_PLUS_EXPR:
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index eda5f05..bca0c59 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2271,6 +2271,7 @@ cp_fold (tree x)
     case FLOAT_EXPR:
     case NEGATE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case BIT_NOT_EXPR:
     case TRUTH_NOT_EXPR:
     case FIXED_CONVERT_EXPR:
diff --git a/gcc/dojump.c b/gcc/dojump.c
index 9da8a0e..88cc96a 100644
--- a/gcc/dojump.c
+++ b/gcc/dojump.c
@@ -467,6 +467,7 @@ do_jump (tree exp, rtx_code_label *if_false_label,
       /* FALLTHRU */
     case NON_LVALUE_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
     case NEGATE_EXPR:
     case LROTATE_EXPR:
     case RROTATE_EXPR:
diff --git a/gcc/expr.c b/gcc/expr.c
index 00a802c..9efa535 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -9074,6 +9074,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
       return REDUCE_BIT_FIELD (temp);
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       op0 = expand_expr (treeop0, subtarget,
 			 VOIDmode, EXPAND_NORMAL);
       if (modifier == EXPAND_STACK_PARM)
@@ -9085,7 +9086,7 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
 
       /* Unsigned abs is simply the operand.  Testing here means we don't
 	 risk generating incorrect code below.  */
-      if (TYPE_UNSIGNED (type))
+      if (TYPE_UNSIGNED (TREE_TYPE (treeop0)))
 	return op0;
 
       return expand_abs (mode, op0, target, unsignedp,
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index faa184a..c19614e 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -1723,7 +1723,8 @@ const_unop (enum tree_code code, tree type, tree arg0)
       && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
       && REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg0))
       && code != NEGATE_EXPR
-      && code != ABS_EXPR)
+      && code != ABS_EXPR
+      && code != ABSU_EXPR)
     return NULL_TREE;
 
   switch (code)
@@ -1758,6 +1759,7 @@ const_unop (enum tree_code code, tree type, tree arg0)
       }
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       if (TREE_CODE (arg0) == INTEGER_CST || TREE_CODE (arg0) == REAL_CST)
 	return fold_abs_const (arg0, type);
       break;
@@ -13846,20 +13848,21 @@ 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));
-	  }
+	  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;
+    break;
 
     case REAL_CST:
       if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg0)))
diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
index afe0147..4fa992d 100644
--- a/gcc/gimple-pretty-print.c
+++ b/gcc/gimple-pretty-print.c
@@ -358,14 +358,17 @@ dump_unary_rhs (pretty_printer *buffer, gassign *gs, int spc,
       break;
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       if (flags & TDF_GIMPLE)
 	{
-	  pp_string (buffer, "__ABS ");
+	  pp_string (buffer,
+		     rhs_code == ABS_EXPR ? "__ABS " : "__ABSU ");
 	  dump_generic_node (buffer, rhs, spc, flags, false);
 	}
       else
 	{
-	  pp_string (buffer, "ABS_EXPR <");
+	  pp_string (buffer,
+		     rhs_code == ABS_EXPR ? "ABS_EXPR <" : "ABSU_EXPR <");
 	  dump_generic_node (buffer, rhs, spc, flags, false);
 	  pp_greater (buffer);
 	}
diff --git a/gcc/gimple-ssa-backprop.c b/gcc/gimple-ssa-backprop.c
index 27aa575..a38b5eb 100644
--- a/gcc/gimple-ssa-backprop.c
+++ b/gcc/gimple-ssa-backprop.c
@@ -405,6 +405,7 @@ backprop::process_assign_use (gassign *assign, tree rhs, usage_info *info)
   switch (gimple_assign_rhs_code (assign))
     {
     case ABS_EXPR:
+    case ABSU_EXPR:
       /* The sign of the input doesn't matter.  */
       info->flags.ignore_sign = true;
       break;
@@ -681,6 +682,7 @@ strip_sign_op_1 (tree rhs)
     switch (gimple_assign_rhs_code (assign))
       {
       case ABS_EXPR:
+      case ABSU_EXPR:
       case NEGATE_EXPR:
 	return gimple_assign_rhs1 (assign);
 
diff --git a/gcc/match.pd b/gcc/match.pd
index 7033730..ba52bb0 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -90,6 +90,17 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 (match (nop_convert @0)
  @0) 
 
+/* Transform likes of (char) ABS_EXPR <(int) x> into (char) ABSU_EXPR <x>
+   ABSU_EXPR returns unsigned absolute value of the operand and the operand
+   of the ABSU_EXPR will have the corresponding signed type.  */
+(simplify (abs (convert @0))
+ (if (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
+      && !TYPE_UNSIGNED (TREE_TYPE (@0))
+      && element_precision (type) > element_precision (TREE_TYPE (@0)))
+  (with { tree utype = unsigned_type_for (TREE_TYPE (@0)); }
+   (convert (absu:utype @0)))))
+
+
 /* Simplifications of operations with one constant operand and
    simplifications to constants or single values.  */
 
diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c
index 71e172c..aa119ec 100644
--- a/gcc/optabs-tree.c
+++ b/gcc/optabs-tree.c
@@ -237,6 +237,8 @@ optab_for_tree_code (enum tree_code code, const_tree type,
     case ABS_EXPR:
       return trapv ? absv_optab : abs_optab;
 
+    case ABSU_EXPR:
+      return abs_optab;
     default:
       return unknown_optab;
     }
diff --git a/gcc/testsuite/gcc.dg/absu.c b/gcc/testsuite/gcc.dg/absu.c
index e69de29..063da28 100644
--- a/gcc/testsuite/gcc.dg/absu.c
+++ b/gcc/testsuite/gcc.dg/absu.c
@@ -0,0 +1,85 @@
+
+/* { dg-do run  } */
+/* { dg-options "-O0" } */
+
+#include <limits.h>
+#define ABS(x)	(((x) >= 0) ? (x) : -(x))
+
+#define DEF_TEST(TYPE)	\
+void foo_##TYPE (signed TYPE x, unsigned TYPE y){	\
+    TYPE t = ABS (x);				\
+    if (t != y)					\
+ 	__builtin_abort ();			\
+}						\
+
+DEF_TEST (char);
+DEF_TEST (short);
+DEF_TEST (int);
+DEF_TEST (long);
+
+int main ()
+{
+  foo_char (SCHAR_MIN + 1, SCHAR_MAX);
+  foo_char (0, 0);
+  foo_char (-1, 1);
+  foo_char (1, 1);
+  foo_char (SCHAR_MAX, SCHAR_MAX);
+
+  foo_int (-1, 1);
+  foo_int (0, 0);
+  foo_int (INT_MAX, INT_MAX);
+  foo_int (INT_MIN + 1, INT_MAX);
+
+  foo_short (-1, 1);
+  foo_short (0, 0);
+  foo_short (SHRT_MAX, SHRT_MAX);
+  foo_short (SHRT_MIN + 1, SHRT_MAX);
+
+  foo_long (-1, 1);
+  foo_long (0, 0);
+  foo_long (LONG_MAX, LONG_MAX);
+  foo_long (LONG_MIN + 1, LONG_MAX);
+
+  return 0;
+}
+
+/* { dg-do run  } */
+/* { dg-options "-O0" } */
+
+#include <limits.h>
+#define ABS(x)	(((x) >= 0) ? (x) : -(x))
+
+#define DEF_TEST(TYPE)	\
+void foo_##TYPE (signed TYPE x, unsigned TYPE y){	\
+    TYPE t = ABS (x);				\
+    if (t != y)					\
+ 	__builtin_abort ();			\
+}						\
+
+DEF_TEST (char);
+DEF_TEST (short);
+DEF_TEST (int);
+DEF_TEST (long);
+void main ()
+{
+  foo_char (SCHAR_MIN + 1, SCHAR_MAX);
+  foo_char (0, 0);
+  foo_char (-1, 1);
+  foo_char (1, 1);
+  foo_char (SCHAR_MAX, SCHAR_MAX);
+
+  foo_int (-1, 1);
+  foo_int (0, 0);
+  foo_int (INT_MAX, INT_MAX);
+  foo_int (INT_MIN + 1, INT_MAX);
+
+  foo_short (-1, 1);
+  foo_short (0, 0);
+  foo_short (SHRT_MAX, SHRT_MAX);
+  foo_short (SHRT_MIN + 1, SHRT_MAX);
+
+  foo_long (-1, 1);
+  foo_long (0, 0);
+  foo_long (LONG_MAX, LONG_MAX);
+  foo_long (LONG_MIN + 1, LONG_MAX);
+}
diff --git a/gcc/testsuite/gcc.dg/gimplefe-29.c b/gcc/testsuite/gcc.dg/gimplefe-29.c
index e69de29..54b86ef 100644
--- a/gcc/testsuite/gcc.dg/gimplefe-29.c
+++ b/gcc/testsuite/gcc.dg/gimplefe-29.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fgimple -fdump-tree-ssa-gimple" } */
+
+unsigned int __GIMPLE() f(int a)
+{
+  unsigned int t0;
+  t0_1 = __ABSU a;
+  return t0_1;
+}
+
+/* { dg-final { scan-tree-dump "__ABSU a" "ssa" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/pr64946.c b/gcc/testsuite/gcc.target/aarch64/pr64946.c
index e69de29..736656f 100644
--- a/gcc/testsuite/gcc.target/aarch64/pr64946.c
+++ b/gcc/testsuite/gcc.target/aarch64/pr64946.c
@@ -0,0 +1,13 @@
+
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+signed char a[100],b[100];
+void absolute_s8 (void)
+{
+  int i;
+  for (i=0; i<16; i++)
+    a[i] = (b[i] > 0 ? b[i] : -b[i]);
+};
+
+/* { dg-final { scan-assembler-times "abs\tv\[0-9\]+.16b, v\[0-9\]+.16b" 1 } } */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 7f48d2d..6df42cc 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))
+	return true;
+      return false;
+      break;
 
     case VEC_DUPLICATE_EXPR:
       if (TREE_CODE (lhs_type) != VECTOR_TYPE
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 3609bca..da87466 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -2471,6 +2471,10 @@ operation_could_trap_helper_p (enum tree_code op,
 	return true;
       return false;
 
+    case ABSU_EXPR:
+      /* ABSU_EXPR never traps.  */
+      return false;
+
     case PLUS_EXPR:
     case MINUS_EXPR:
     case MULT_EXPR:
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 5a0a252..d272974 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3866,6 +3866,7 @@ estimate_operator_cost (enum tree_code code, eni_weights *weights,
     case MIN_EXPR:
     case MAX_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
 
     case LSHIFT_EXPR:
     case RSHIFT_EXPR:
diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
index bc36c28..612a18f 100644
--- a/gcc/tree-pretty-print.c
+++ b/gcc/tree-pretty-print.c
@@ -2463,6 +2463,12 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags,
       pp_greater (pp);
       break;
 
+    case ABSU_EXPR:
+      pp_string (pp, "ABSU_EXPR <");
+      dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false);
+      pp_greater (pp);
+      break;
+
     case RANGE_EXPR:
       NIY;
       break;
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 5c2578f..e4df6c8 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -614,7 +614,8 @@ vect_recog_sad_pattern (vec<gimple *> *stmts, tree *type_in,
   gcc_assert (abs_stmt_vinfo);
   if (STMT_VINFO_DEF_TYPE (abs_stmt_vinfo) != vect_internal_def)
     return NULL;
-  if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR)
+  if (gimple_assign_rhs_code (abs_stmt) != ABS_EXPR
+      && gimple_assign_rhs_code (abs_stmt) != ABSU_EXPR)
     return NULL;
 
   tree abs_oprnd = gimple_assign_rhs1 (abs_stmt);
diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
index 4539f6a..c71b688 100644
--- a/gcc/tree-vect-stmts.c
+++ b/gcc/tree-vect-stmts.c
@@ -5995,7 +5995,10 @@ 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, vectype_out);
+  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
diff --git a/gcc/tree.def b/gcc/tree.def
index 31de6c0..a1766e4 100644
--- a/gcc/tree.def
+++ b/gcc/tree.def
@@ -761,6 +761,11 @@ DEFTREECODE (MAX_EXPR, "max_expr", tcc_binary, 2)
    operand of the ABS_EXPR must have the same type.  */
 DEFTREECODE (ABS_EXPR, "abs_expr", tcc_unary, 1)
 
+/* 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.  */
+DEFTREECODE (ABSU_EXPR, "absu_expr", tcc_unary, 1)
+
 /* Shift operations for shift and rotate.
    Shift means logical shift if done on an
    unsigned type, arithmetic shift if done on a signed type.
Richard Biener June 4, 2018, 8:38 a.m. | #8
On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
>

> Hi Richard,

>

> Thanks for the review.

>

> On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote:

> > On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah

> > <kugan.vivekanandarajah@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.vivekanandarajah@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.guenther@gmail.com> wrote:

> >> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

> >> >>

> >> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

> >> >>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

> >> >>> >

> >> >>> >     * gcc.dg/absu.c: New test.
Kugan Vivekanandarajah June 11, 2018, 8:27 a.m. | #9
Hi Richard,

Thanks for the review and sorry for getting back to you late.

On 4 June 2018 at 18:38, Richard Biener <richard.guenther@gmail.com> wrote:
> On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah

> <kugan.vivekanandarajah@linaro.org> wrote:

>>

>> Hi Richard,

>>

>> Thanks for the review.

>>

>> On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote:

>> > On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah

>> > <kugan.vivekanandarajah@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);


Some testcases are failing with the changes.

gcc:gcc.dg/tree-ssa/pr83329.c (internal compiler error) : This seems
to be due to POINTER_DIFF_EXPR (?)

There is also

gcc:gcc.target/i386/avx2-vshift-1.c (internal compiler error)
gcc:gcc.target/i386/xop-vshift-1.c (internal compiler error)

Example:
/home/tcwg-buildslave/workspace/tcwg-buildfarm_2/tcwg-x86_32-build/snapshots/gcc.git~master_rev_bfa6b77/gcc/testsuite/gcc.target/i386/xop-vshift-1.c:73:1:
error: type mismatch in binary expression^M
vector(4) long long unsigned int^M
^M
vector(4) long long int^M
^M
vector(4) long long int^M
^M
vect_patt_17.54_21 = vect__2.53_16 & vect_cst__20;^M
during GIMPLE pass: vect^M

Thanks,
Kugan



>

> 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.vivekanandarajah@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.guenther@gmail.com> wrote:

>> >> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

>> >> >>

>> >> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

>> >> >>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

>> >> >>> >

>> >> >>> >     * gcc.dg/absu.c: New test.
Richard Biener June 13, 2018, 12:07 p.m. | #10
On Mon, Jun 11, 2018 at 10:28 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
>

> Hi Richard,

>

> Thanks for the review and sorry for getting back to you late.

>

> On 4 June 2018 at 18:38, Richard Biener <richard.guenther@gmail.com> wrote:

> > On Mon, Jun 4, 2018 at 10:18 AM Kugan Vivekanandarajah

> > <kugan.vivekanandarajah@linaro.org> wrote:

> >>

> >> Hi Richard,

> >>

> >> Thanks for the review.

> >>

> >> On 1 June 2018 at 22:20, Richard Biener <richard.guenther@gmail.com> wrote:

> >> > On Fri, Jun 1, 2018 at 4:12 AM Kugan Vivekanandarajah

> >> > <kugan.vivekanandarajah@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);

>

> Some testcases are failing with the changes.

>

> gcc:gcc.dg/tree-ssa/pr83329.c (internal compiler error) : This seems

> to be due to POINTER_DIFF_EXPR (?)


Ah, yes.  I'd simply handle that in a more clear way.

> There is also

>

> gcc:gcc.target/i386/avx2-vshift-1.c (internal compiler error)

> gcc:gcc.target/i386/xop-vshift-1.c (internal compiler error)

>

> Example:

> /home/tcwg-buildslave/workspace/tcwg-buildfarm_2/tcwg-x86_32-build/snapshots/gcc.git~master_rev_bfa6b77/gcc/testsuite/gcc.target/i386/xop-vshift-1.c:73:1:

> error: type mismatch in binary expression^M

> vector(4) long long unsigned int^M

> ^M

> vector(4) long long int^M

> ^M

> vector(4) long long int^M

> ^M

> vect_patt_17.54_21 = vect__2.53_16 & vect_cst__20;^M

> during GIMPLE pass: vect^M


That one is odd and looks like a latent bug in pattern creation:

diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 507c5b94f07..6786ffcd4c6 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -2185,6 +2185,11 @@ vect_recog_vector_vector_shift_pattern
(vec<gimple *> *stmts,
                                       TYPE_PRECISION (TREE_TYPE (oprnd1)));
              def = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
              def_stmt = gimple_build_assign (def, BIT_AND_EXPR, rhs1, mask);
+             stmt_vec_info new_stmt_info
+               = new_stmt_vec_info (def_stmt, vinfo);
+             set_vinfo_for_stmt (def_stmt, new_stmt_info);
+             STMT_VINFO_VECTYPE (new_stmt_info)
+               = get_vectype_for_scalar_type (TREE_TYPE (rhs1));
              new_pattern_def_seq (stmt_vinfo, def_stmt);
            }
        }


I can see if I can make this change independently of yours (that is,
I'm bootstrapping
and testing the above together with an equivalent vectorizable_operation hunk).

Richard.

>

> Thanks,

> Kugan

>

>

>

> >

> > 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.vivekanandarajah@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.guenther@gmail.com> wrote:

> >> >> >> On Thu, May 17, 2018 at 4:56 AM Andrew Pinski <pinskia@gmail.com> wrote:

> >> >> >>

> >> >> >>> On Wed, May 16, 2018 at 7:14 PM, Kugan Vivekanandarajah

> >> >> >>> <kugan.vivekanandarajah@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.vivekanandarajah@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.vivekanandarajah@linaro.org>

> >> >> >>> >

> >> >> >>> >     * gcc.dg/absu.c: New test.

Patch

diff --git a/gcc/expr.c b/gcc/expr.c
index 5e3d9a5..67f8dd1 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -9063,6 +9063,7 @@  expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
       return REDUCE_BIT_FIELD (temp);
 
     case ABS_EXPR:
+    case ABSU_EXPR:
       op0 = expand_expr (treeop0, subtarget,
 			 VOIDmode, EXPAND_NORMAL);
       if (modifier == EXPAND_STACK_PARM)
@@ -9074,7 +9075,8 @@  expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
 
       /* 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;
 
       return expand_abs (mode, op0, target, unsignedp,
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 3a99b66..6e80178 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -5324,8 +5324,17 @@  fold_cond_expr_with_comparison (location_t loc, tree type,
       case GT_EXPR:
 	if (TYPE_UNSIGNED (TREE_TYPE (arg1)))
 	  break;
-	tem = fold_build1_loc (loc, ABS_EXPR, TREE_TYPE (arg1), arg1);
-	return fold_convert_loc (loc, type, tem);
+	if (TREE_CODE (arg1) == NOP_EXPR)
+	  {
+	    arg1 = TREE_OPERAND (arg1, 0);
+	    tem = fold_build1_loc (loc, ABSU_EXPR, unsigned_type_for (arg1_type), arg1);
+	    return fold_convert_loc (loc, type, tem);
+	  }
+	else
+	  {
+	    tem = fold_build1_loc (loc, ABS_EXPR, TREE_TYPE (arg1), arg1);
+	    return fold_convert_loc (loc, type, tem);
+	  }
       case UNLE_EXPR:
       case UNLT_EXPR:
 	if (flag_trapping_math)
@@ -7698,7 +7707,8 @@  fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0)
   if (arg0)
     {
       if (CONVERT_EXPR_CODE_P (code)
-	  || code == FLOAT_EXPR || code == ABS_EXPR || code == NEGATE_EXPR)
+	  || code == FLOAT_EXPR || code == ABS_EXPR
+	  || code == ABSU_EXPR || code == NEGATE_EXPR)
 	{
 	  /* Don't use STRIP_NOPS, because signedness of argument type
 	     matters.  */
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;
 
     default:
diff --git a/gcc/testsuite/gcc.dg/absu.c b/gcc/testsuite/gcc.dg/absu.c
index e69de29..43e651b 100644
--- a/gcc/testsuite/gcc.dg/absu.c
+++ b/gcc/testsuite/gcc.dg/absu.c
@@ -0,0 +1,39 @@ 
+
+/* { dg-do run  } */
+/* { dg-options "-O0" } */
+
+#include <limits.h>
+#define ABS(x)	(((x) >= 0) ? (x) : -(x))
+
+#define DEF_TEST(TYPE)	\
+void foo_##TYPE (signed TYPE x, unsigned TYPE y){	\
+    TYPE t = ABS (x);				\
+    if (t != y)					\
+ 	__builtin_abort ();			\
+}						\
+
+DEF_TEST (char);
+DEF_TEST (short);
+DEF_TEST (int);
+DEF_TEST (long);
+void main ()
+{
+  foo_char (SCHAR_MIN + 1, SCHAR_MAX);
+  foo_char (0, 0);
+  foo_char (SCHAR_MAX, SCHAR_MAX);
+
+  foo_int (-1, 1);
+  foo_int (0, 0);
+  foo_int (INT_MAX, INT_MAX);
+  foo_int (INT_MIN + 1, INT_MAX);
+
+  foo_short (-1, 1);
+  foo_short (0, 0);
+  foo_short (SHRT_MAX, SHRT_MAX);
+  foo_short (SHRT_MIN + 1, SHRT_MAX);
+
+  foo_long (-1, 1);
+  foo_long (0, 0);
+  foo_long (LONG_MAX, LONG_MAX);
+  foo_long (LONG_MIN + 1, LONG_MAX);
+}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 9485f73..59a115c 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -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:
     case BIT_FIELD_REF:
@@ -3937,6 +3940,9 @@  verify_gimple_assign_unary (gassign *stmt)
     case PAREN_EXPR:
     case CONJ_EXPR:
       break;
+    case ABSU_EXPR:
+      /* FIXME.  */
+      return false;
 
     case VEC_DUPLICATE_EXPR:
       if (TREE_CODE (lhs_type) != VECTOR_TYPE
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 71dac4f..13d4c25 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -466,7 +466,8 @@  fold_build_cond_expr (tree type, tree cond, tree rhs, tree lhs)
   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);
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 5a0a252..d272974 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3866,6 +3866,7 @@  estimate_operator_cost (enum tree_code code, eni_weights *weights,
     case MIN_EXPR:
     case MAX_EXPR:
     case ABS_EXPR:
+    case ABSU_EXPR:
 
     case LSHIFT_EXPR:
     case RSHIFT_EXPR:
diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
index 276ad00..b74d8c9 100644
--- a/gcc/tree-pretty-print.c
+++ b/gcc/tree-pretty-print.c
@@ -2460,6 +2460,12 @@  dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags,
       pp_greater (pp);
       break;
 
+    case ABSU_EXPR:
+      pp_string (pp, "ABSU_EXPR <");
+      dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false);
+      pp_greater (pp);
+      break;
+
     case RANGE_EXPR:
       NIY;
       break;
diff --git a/gcc/tree.def b/gcc/tree.def
index 31de6c0..768167b 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