Fix ICE in re-simplification of VEC_COND_EXPR (was: Re: [PATCH][amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR)

Hi,

On 29.11.19 13:51, Harwath, Frederik wrote:

>> condition for the inner vec_cond.  Your fix looks reasonable but is
>> very badly formatted.  Can you instead do

I hope the formatting looks better now. I have also removed the [amdgcn] from the subject line since
the fact that this has been discovered in the context of amdgcn is not really essential.

Best regards,
Frederik


2019-11-29  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 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

On Fri, Nov 29, 2019 at 02:38:34PM +0100, Harwath, Frederik wrote:
> 2019-11-29  Frederik Harwath  <frederik@codesourcery.com>
> 
> gcc/
> 	* gimple-match-head.c (maybe_resimplify_conditional_op): use

s/use/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 | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/gimple-match-head.c b/gcc/gimple-match-head.c
> index 2996bade301..c763a80a6d1 100644
> --- a/gcc/gimple-match-head.c
> +++ b/gcc/gimple-match-head.c
> @@ -144,9 +144,21 @@ 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;
> +      bool op_could_trap;
> +
> +      /* 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

s/. /.  /

> +	  don't need to consider the operands. */

Likewise.

	Jakub

Hi Jakub,

On 29.11.19 14:41, Jakub Jelinek wrote:

> s/use/Use/
>
> [...]
>
> s/. /.  /

Right, thanks. Does that look ok for inclusion in trunk now?

Best regards,
Frederik


2019-11-29  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 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/gcc/gimple-match-head.c b/gcc/gimple-match-head.c
index 2996bade301..9010f11621e 100644
--- a/gcc/gimple-match-head.c
+++ b/gcc/gimple-match-head.c
@@ -144,9 +144,21 @@ 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;
+      bool op_could_trap;
+
+      /* 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.  */
+      if (op_code == COND_EXPR || op_code == VEC_COND_EXPR)
+       op_could_trap = generic_expr_could_trap_p (res_op->ops[0]);
+      else
+       op_could_trap = 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;

"Harwath, Frederik" <frederik@codesourcery.com> writes:
> Hi Jakub,
>
> On 29.11.19 14:41, Jakub Jelinek wrote:
>
>> s/use/Use/
>>
>> [...]
>>
>> s/. /.  /
>
> Right, thanks. Does that look ok for inclusion in trunk now?
>
> Best regards,
> Frederik
>
>
> 2019-11-29  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.

Thanks for doing this, looks good to me FWIW.  I was seeing the same
failure for SVE but hadn't found time to look at it.

Richard

> ---
>  gcc/gimple-match-head.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/gimple-match-head.c b/gcc/gimple-match-head.c
> index 2996bade301..9010f11621e 100644
> --- a/gcc/gimple-match-head.c
> +++ b/gcc/gimple-match-head.c
> @@ -144,9 +144,21 @@ 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;
> +      bool op_could_trap;
> +
> +      /* 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.  */
> +      if (op_code == COND_EXPR || op_code == VEC_COND_EXPR)
> +       op_could_trap = generic_expr_could_trap_p (res_op->ops[0]);
> +      else
> +       op_could_trap = 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;

On 29.11.19 15:46, Richard Sandiford wrote:

> Thanks for doing this, looks good to me FWIW.  I was seeing the same
> failure for SVE but hadn't found time to look at it.

Thank you all for the review. Committed as r278853.

Frederik