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

Message ID 73fbd025-06d0-0641-00e2-a47bd8879e6a@codesourcery.com
State New
Headers show
Series
  • Fix ICE in re-simplification of VEC_COND_EXPR (was: Re: [PATCH][amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR)
Related show

Commit Message

Harwath, Frederik Nov. 29, 2019, 1:38 p.m.
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(-)

-- 
2.17.1

Comments

Jakub Jelinek Nov. 29, 2019, 1:41 p.m. | #1
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
Harwath, Frederik Nov. 29, 2019, 1:56 p.m. | #2
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;
-- 
2.17.1
Richard Sandiford Nov. 29, 2019, 2:46 p.m. | #3
"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;
Harwath, Frederik Nov. 29, 2019, 3:09 p.m. | #4
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

Patch

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