[amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR

Hi,
currently, on trunk, the tests gcc.dg/vect/vect-cond-reduc-1.c and gcc.dg/pr68286.c fail when compiling for amdgcn-unknown-amdhsa.
The reason seems to lie in the interaction of the changes that have been introduced by revision r276659
("Allow COND_EXPR and VEC_COND_EXPR condtions to trap" by Ilya Leoshkevich) of trunk and the vectorized code that is generated for amdgcn.

If the function maybe_resimplify_conditional_op from gimple-match-head.c gets called on a conditional operation without an "else" part, it
makes the operation unconditional, but only if the operation cannot trap. To check this, it uses operation_could_trap_p.
This ends up in a violated assertion in the latter function if maybe_resimplify_conditional_op is called on a COND_EXPR or VEC_COND_EXPR:

 /* This function cannot tell whether or not COND_EXPR and VEC_COND_EXPR could
     trap, because that depends on the respective condition op.  */
  gcc_assert (op != COND_EXPR && op != VEC_COND_EXPR);

A related issue has been resolved by the patch that was committed as r276915 ("PR middle-end/92063" by Jakub Jelinek).

In our case, the error is triggered by the simplification rule at line 3450 of gcc/match.pd:

/* A + (B vcmp C ? 1 : 0) -> A - (B vcmp C ? -1 : 0), since vector comparisons
   return all -1 or all 0 results.  */
/* ??? We could instead convert all instances of the vec_cond to negate,
   but that isn't necessarily a win on its own.  */
(simplify
 (plus:c @3 (view_convert? (vec_cond:s @0 integer_each_onep@1 integer_zerop@2)))
 (if (VECTOR_TYPE_P (type)
      && known_eq (TYPE_VECTOR_SUBPARTS (type),
		   TYPE_VECTOR_SUBPARTS (TREE_TYPE (@1)))
      && (TYPE_MODE (TREE_TYPE (type))
          == TYPE_MODE (TREE_TYPE (TREE_TYPE (@1)))))
  (minus @3 (view_convert (vec_cond @0:0 (negate @1) @2)))))
)
	
It seems that this rule is not invoked when compiling for x86_64 where the generated code for vect-cond-reduc-1.c does not contain anything that would
match this rule. Could it be that there is no test covering this rule for commonly tested architectures?

I have changed maybe_resimplify_conditional_op to check if a COND_EXPR or VEC_COND_EXPR could trap by checking whether the condition can trap using
generic_expr_could_trap_p. Judging from the comment above the assertion and the code changes of r276659, it seems that this is both necessary and
sufficient to verify if those expressions can trap.

Does that sound reasonable and can the patch be included in trunk?

The patch fixes the failing tests for me and does not cause any visible regressions in the results of "make check" which I have executed for targets amdgcn-unknown-amdhsa
and x86_64-pc-linux-gnu.

Best regards,
Frederik



2019-11-28  Frederik Harwath  <frederik@codesourcery.com>

gcc/
	* gimple-match-head.c (maybe_resimplify_conditional_op): use
  	generic_expr_could_trap_p to check if the condition of COND_EXPR or
  	VEC_COND_EXPR can trap.
---
 gcc/gimple-match-head.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

On Fri, Nov 29, 2019 at 1:24 PM Harwath, Frederik
<frederik@codesourcery.com> wrote:
>
> Hi,
> currently, on trunk, the tests gcc.dg/vect/vect-cond-reduc-1.c and gcc.dg/pr68286.c fail when compiling for amdgcn-unknown-amdhsa.
> The reason seems to lie in the interaction of the changes that have been introduced by revision r276659
> ("Allow COND_EXPR and VEC_COND_EXPR condtions to trap" by Ilya Leoshkevich) of trunk and the vectorized code that is generated for amdgcn.
>
> If the function maybe_resimplify_conditional_op from gimple-match-head.c gets called on a conditional operation without an "else" part, it
> makes the operation unconditional, but only if the operation cannot trap. To check this, it uses operation_could_trap_p.
> This ends up in a violated assertion in the latter function if maybe_resimplify_conditional_op is called on a COND_EXPR or VEC_COND_EXPR:
>
>  /* This function cannot tell whether or not COND_EXPR and VEC_COND_EXPR could
>      trap, because that depends on the respective condition op.  */
>   gcc_assert (op != COND_EXPR && op != VEC_COND_EXPR);
>
> A related issue has been resolved by the patch that was committed as r276915 ("PR middle-end/92063" by Jakub Jelinek).
>
> In our case, the error is triggered by the simplification rule at line 3450 of gcc/match.pd:
>
> /* A + (B vcmp C ? 1 : 0) -> A - (B vcmp C ? -1 : 0), since vector comparisons
>    return all -1 or all 0 results.  */
> /* ??? We could instead convert all instances of the vec_cond to negate,
>    but that isn't necessarily a win on its own.  */
> (simplify
>  (plus:c @3 (view_convert? (vec_cond:s @0 integer_each_onep@1 integer_zerop@2)))
>  (if (VECTOR_TYPE_P (type)
>       && known_eq (TYPE_VECTOR_SUBPARTS (type),
>                    TYPE_VECTOR_SUBPARTS (TREE_TYPE (@1)))
>       && (TYPE_MODE (TREE_TYPE (type))
>           == TYPE_MODE (TREE_TYPE (TREE_TYPE (@1)))))
>   (minus @3 (view_convert (vec_cond @0:0 (negate @1) @2)))))
> )
>
> It seems that this rule is not invoked when compiling for x86_64 where the generated code for vect-cond-reduc-1.c does not contain anything that would
> match this rule. Could it be that there is no test covering this rule for commonly tested architectures?

This was all added for aarch64 SVE.  So it looks like the outer plus
was conditional and we end up inheriting the
condition for the inner vec_cond.  Your fix looks reasonable but is
very badly formatted.  Can you instead do

 if (op_Code == cOND_EPXR || op_code == vEC_COND_EXPR)
   op_could_trap = generic_expr_could_trap (..)
 else
  op_could_trap = operation_could_trap_p (...

Thanks,
RIchard.

> I have changed maybe_resimplify_conditional_op to check if a COND_EXPR or VEC_COND_EXPR could trap by checking whether the condition can trap using
> generic_expr_could_trap_p. Judging from the comment above the assertion and the code changes of r276659, it seems that this is both necessary and
> sufficient to verify if those expressions can trap.
>
> Does that sound reasonable and can the patch be included in trunk?
>
> The patch fixes the failing tests for me and does not cause any visible regressions in the results of "make check" which I have executed for targets amdgcn-unknown-amdhsa
> and x86_64-pc-linux-gnu.
>
> Best regards,
> Frederik
>
>
>
> 2019-11-28  Frederik Harwath  <frederik@codesourcery.com>
>
> gcc/
>         * gimple-match-head.c (maybe_resimplify_conditional_op): use
>         generic_expr_could_trap_p to check if the condition of COND_EXPR or
>         VEC_COND_EXPR can trap.
> ---
>  gcc/gimple-match-head.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/gimple-match-head.c b/gcc/gimple-match-head.c
> index 2996bade301..4da6c4d7458 100644
> --- a/gcc/gimple-match-head.c
> +++ b/gcc/gimple-match-head.c
> @@ -144,9 +144,17 @@ maybe_resimplify_conditional_op (gimple_seq *seq, gimple_match_op *res_op,
>        /* Likewise if the operation would not trap.  */
>        bool honor_trapv = (INTEGRAL_TYPE_P (res_op->type)
>                           && TYPE_OVERFLOW_TRAPS (res_op->type));
> -      if (!operation_could_trap_p ((tree_code) res_op->code,
> -                                  FLOAT_TYPE_P (res_op->type),
> -                                  honor_trapv, res_op->op_or_null (1)))
> +      tree_code op_code = (tree_code) res_op->code;
> +      /* COND_EXPR and VEC_COND_EXPR will trap if, and only if, the condition
> +        traps and hence we have to check this. For all other operations, we
> +        don't need to consider the operands. */
> +      bool op_could_trap = op_code == COND_EXPR || op_code == VEC_COND_EXPR ?
> +       generic_expr_could_trap_p (res_op->ops[0]) :
> +       operation_could_trap_p ((tree_code) res_op->code,
> +                               FLOAT_TYPE_P (res_op->type),
> +                               honor_trapv, res_op->op_or_null (1));
> +
> +      if (!op_could_trap)
>         {
>           res_op->cond.cond = NULL_TREE;
>           return false;
> --
> 2.17.1
>

Hi Richard,

On 29.11.19 13:37, Richard Biener wrote:
> On Fri, Nov 29, 2019 at 1:24 PM Harwath, Frederik
> <frederik@codesourcery.com> wrote:
> [...]
>> It seems that this rule is not invoked when compiling for x86_64 where the generated code for vect-cond-reduc-1.c does not contain anything that would
>> match this rule. Could it be that there is no test covering this rule for commonly tested architectures?
> 
> This was all added for aarch64 SVE.  So it looks like the outer plus
> was conditional and we end up inheriting the
I should have mentioned this, it was indeed a COND_ADD.

> condition for the inner vec_cond.  Your fix looks reasonable but is
> very badly formatted.  Can you instead do
> 
>  if (op_Code == cOND_EPXR || op_code == vEC_COND_EXPR)
>    op_could_trap = generic_expr_could_trap (..)
>  else
>   op_could_trap = operation_could_trap_p (...
> 

Sorry, sure!

Thanks,
Frederik