On Fri, Jun 12, 2020 at 3:24 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 6/12/20 11:43 AM, Richard Biener wrote:
> > So ... how far are you with enforcing a split VEC_COND_EXPR?
> > Thus can we avoid the above completely (even as intermediate
> > state)?
>
> Apparently, I'm quite close. Using the attached patch I see only 2 testsuite
> failures:
>
> FAIL: gcc.dg/tree-ssa/pr68714.c scan-tree-dump-times reassoc1 " <= " 1
> FAIL: gcc.target/i386/pr78102.c scan-assembler-times pcmpeqq 3
>
> The first one is about teaching reassoc about the SSA_NAMEs in VEC_COND_EXPR.
> I haven't
> analyze the second failure.
>
> I'm also not sure about the gimlification change, I see a superfluous
> assignments:
> vec_cond_cmp.5 = _1 == _2;
> vec_cond_cmp.6 = vec_cond_cmp.5;
> vec_cond_cmp.7 = vec_cond_cmp.6;
> _3 = VEC_COND_EXPR <vec_cond_cmp.7, { -1, -1, -1, -1, -1, -1, -1, -1 }, {
> 0, 0, 0, 0, 0, 0, 0, 0 }>;
> ?
>
> So with the suggested patch, the EH should be gone as you suggested. Right?
Right, it should be on the comparison already from the start.
@@ -14221,9 +14221,13 @@ gimplify_expr (tree *expr_p, gimple_seq
*pre_p, gimple_seq *post_p,
case VEC_COND_EXPR:
{
enum gimplify_status r0, r1, r2;
-
r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
post_p, is_gimple_condexpr, fb_rvalue);
+ tree xop0 = TREE_OPERAND (*expr_p, 0);
+ tmp = create_tmp_var_raw (TREE_TYPE (xop0), "vec_cond_cmp");
+ gimple_add_tmp_var (tmp);
+ gimplify_assign (tmp, xop0, pre_p);
+ TREE_OPERAND (*expr_p, 0) = tmp;
r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
post_p, is_gimple_val, fb_rvalue);
all of VEC_COND_EXPR can now be a simple goto expr_3;
diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c
index 494c9e9c20b..090fb52a2f1 100644
--- a/gcc/tree-ssa-forwprop.c
+++ b/gcc/tree-ssa-forwprop.c
@@ -3136,6 +3136,10 @@ pass_forwprop::execute (function *fun)
if (code == COND_EXPR
|| code == VEC_COND_EXPR)
{
+ /* Do not propagate into VEC_COND_EXPRs. */
+ if (code == VEC_COND_EXPR)
+ break;
+
err - remove the || code == VEC_COND_EXPR instead?
@@ -2221,24 +2226,12 @@ expand_vector_operations (void)
{
gimple_stmt_iterator gsi;
basic_block bb;
- bool cfg_changed = false;
FOR_EACH_BB_FN (bb, cfun)
- {
- for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
- {
- expand_vector_operations_1 (&gsi);
- /* ??? If we do not cleanup EH then we will ICE in
- verification. But in reality we have created wrong-code
- as we did not properly transition EH info and edges to
- the piecewise computations. */
- if (maybe_clean_eh_stmt (gsi_stmt (gsi))
- && gimple_purge_dead_eh_edges (bb))
- cfg_changed = true;
- }
- }
I'm not sure about this. Consider the C++ testcase where
the ?: is replaced by a division. If veclower needs to replace
that with four scalrar division statements then the above
still applies - veclower does not correctly duplicate EH info
and EH edges to the individual divisions (and we do not know
which component might trap).
So please leave the above in. You can try if using integer
division makes it break and add such a testcase if there's
no coverage for this in the testsuite.
What's missing from the patch is adjusting
verify_gimple_assign_ternary from
if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
|| !is_gimple_val (rhs2)
|| !is_gimple_val (rhs3))
{
error ("invalid operands in ternary operation");
return true;
to the same with the rhs_code == VEC_COND_EXPR case removed.
You'll likely figure the vectorizer still creates some VEC_COND_EXPRs
with embedded comparisons.
Thanks,
Richard.
> Martin
