Re: [PATCH] Fix ICE in re-simplification of VEC_COND_EXPR (was: Re: [PATCH][amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR)
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
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
Re: [PATCH] Fix ICE in re-simplification of VEC_COND_EXPR (was: Re: [PATCH][amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR)
On Fri, Nov 29, 2019 at 02:38:34PM +0100, Harwath, Frederik wrote: > 2019-11-29 Frederik Harwath > > 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
[PATCH] 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
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..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;
--
2.17.1
Re: [PATCH][amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR
Hi Richard, On 29.11.19 13:37, Richard Biener wrote: > On Fri, Nov 29, 2019 at 1:24 PM Harwath, Frederik > 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
Re: [PATCH][amdgcn] Fix ICE in re-simplification of VEC_COND_EXPR
On Fri, Nov 29, 2019 at 1:24 PM Harwath, Frederik
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
>
> 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
[PATCH][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
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
