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

[Richard Biener <richard dot guenther at gmail dot com>]

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.