On Fri, Nov 29, 2019 at 1:24 PM Harwath, Frederik
<frede...@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 <frede...@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
>
