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

[Kugan Vivekanandarajah]

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.