This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][RFC] Drop GENERIC conds in [VEC_]COND_EXPRs
On Wed, 6 Jun 2018, Richard Sandiford wrote:
> Richard Biener <rguenther@suse.de> writes:
> > On Tue, 5 Jun 2018, Richard Biener wrote:
> >
> >>
> >> This is a prototype enforcing is_gimple_val conditions in
> >> [VEC_]COND_EXPRs. It shows quite some fallout - some likely
> >> due to necessary testsuite adjustments like gcc.dg/tree-ssa/ssa-lim-12.c
> >> others because some code simply doesn't handle conditions visible
> >> only via following SSA defs like vect_recog_mixed_size_cond_pattern.
> >>
> >> So on x86_64 there's quite some vect.exp fallout.
> >>
> >> Anyhow, to assess the effect on other targets I'm throwing this
> >> in here.
> >>
> >> I've hopefully nailed all ICEs (or at least makes catching them
> >> easy with some asserts in GIMPLE stmt building).
> >>
> >> Full bootstrap and regtest for all languages running on
> >> x86_64-unknown-linux-gnu.
>
> FWIW, I ran the original version through our internal SVE benchmarks.
> There were quite a few ICEs from:
>
> pattern_stmt
> = gimple_build_assign (vect_recog_temp_ssa_var (itype, NULL),
> COND_EXPR, cond_expr, trueval,
> build_int_cst (itype, 0));
>
> in adjust_bool_pattern and some from loop_cand::undo_simple_reduction
> (which it looks like you fixed in the updated patch). But of the
> tests that successfully built, it looks like this is neutral (good!).
>
> I can try to fix the adjust_bool_pattern thing if you don't see it for AVX.
Didn't run into it indeed. A fix would probably look like
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 1418c83690b..507c5b94f07 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -3484,7 +3484,9 @@ adjust_bool_pattern (tree var, tree out_type,
}
else
itype = TREE_TYPE (rhs1);
- cond_expr = build2_loc (loc, rhs_code, itype, rhs1, rhs2);
+ cond_expr = make_ssa_name (itype);
+ pattern_stmt = gimple_build_assign (cond_expr, rhs_code, rhs1,
rhs2);
+ append_pattern_def_seq (stmt_info, pattern_stmt);
if (trueval == NULL_TREE)
trueval = build_int_cst (itype, 1);
else
included in the updated version below.
> > Testing shows a few omissions in the patch (updated patch attached
> > below). It also shows that expand_vec_cond_expr_p and friends
> > need some heavy lifting to deal with vcond* patterns which have
> > the comparison embedded. There's vcond_mask* which would be
> > more suited to the GIMPLE we have after this patch and I suspect
> > we can always expand
> >
> > _1 = _2 < _3;
> > _4 = _1 ? _5 : _66;
> >
> > as
> >
> > _1 = _2 < _3 ? { -1,...} : {0,...}; via vcond_*
> > _4 = _1 != 0 ? _5 : _6; via vcond_*
> >
> > at expansion time TER might come to the rescue, if there are multiple
> > uses of _1 then there's cost issues to honor in case _1 = _2 < _3
> > can be expanded more efficiently than via such fake vcond_*.
> >
> > In any case the expander now needs to be made more smart
> > (as well as the pattern recog part of the vectorizer).
>
> Why not match them as internal functions, like we now do for FMA* etc.?
> It seems like a similar case: for FMAs we wanted to fold in negates while
> here we want to fold in the comparison. The fold could be handled by
> match.pd, gated on a suitably late predicate. (Unless the vectoriser
> really does have to generate it directly, in which case maybe it could
> also/instead be done as a pattern. That might even make the generation
> side simpler, since the pattern would just go through vectorizable_call.)
I think the vectorizer needs to generate it directly since the invariant
is it emits code the targets can actually handle (so tree-vect-generic.c
doesn't undo things). So that would basically mean to scrap
VEC_COND_EXPR and instead have direct optabs IFNs for
vcond*_* and eventually vec_cmp*. Of course the disadvantage is that
we need to handle those in followup optimiziation (we're not too good
in generating optimal predicated code...).
> Using one internal function per comparison type would also help to clean
> up the optabs interface. At the moment, targets basically have to
> accept any comparison type that's going, regardless of what the target
> can actually support. If we use internal functions that directly map to
> individual optabs, then we can use target-independent code to make the
> comparison fit. (At least for the obvious cases. Some FP comparisons
> might still be better emulated directly by the target.)
We could try at least handling the TER/combine issue by a late
pass doing the matching to IFNs for the vcond*_* optabs. Or have
tree-vect-generic.c do it (and lower the rest). Still the
vectorizer needs to make sure to generate only target supported
stuff - not sure how good of a job it does here given later
optimizations might mangle the GIMPLE and we're only careful
when folding VEC_PERM_EXPRs.
> > Jakub - I remember you weren't too happy about splitting the
> > conditions out of [VEC_]COND_EXPRs this came up last time?
> > With the alternative idea of transitioning [VEC_]COND_EXPRs to
> > tcc_comparisons with conditional ops aka
> >
> > _1 = _2 < _3 ? _5 : _6;
> >
> > with RHS code LT_EXPR and 4 operands what do you think of
> > Richards complaint about being forced to repeat _2 < _3?
> > A split out _2 < _3 would then look like
> >
> > _4 = _2 < _3 ? {-1,...} : {0,...};
> > _1 = _4 != {0,...} ? _5 : _6;
> >
> > so "plain" LT_EXPR rhs wouldn't be supported anymore but they'd
> > always have explicit true and false values attached. With making
> > the last two ops optional we could make them implicit again of course.
> > We could also keep [VEC_]COND_EXPR then in 3-operand form with
> > a gimple-val predicate rather than a comparison. Of course that
> > wouldn't really simplify the IL overall...
>
> Yeah, this still doesn't seem like an improvement to me TBH. For one
> thing it'd be confusing for LT_EXPR to have 2 operands for generic and
> 4 for gimple: are there any other cases where we do that? I suppose the
> GIMPLE_SINGLE_RHS stuff is a special case too, but it sounded like you
> saw that was a wart and wanted to move away from it.
Aww, didn't think of GENERIC here ;)
But yes, just doing the splitting out of the GENERIC comparison
op of [VEC_]COND_EXPR and somehow dealing with the fallout would be
my preference at this point. I'll work some more of this after
I return from a short vacation mid next week. At least if we can
agree on that simple plan.
GIMPLE_SINGLE_RHS stuff is special but unrelated. There's currently
only WITH_SIZE_EXPR and OBJ_TYPE_REF that could be moved out
but IIRC those only appear in operand context and not standalone.
Note that all this rhs-class stuff is really a useless indirection
over the num_ops interface we already have... (or over the
tree code class you can query from the rhs code). I'd love to
get rid of it ;)
Richard.
>From b3785e1148f2dbbbac82d544ea4efe09d25378de Mon Sep 17 00:00:00 2001
From: Richard Guenther <rguenther@suse.de>
Date: Tue, 5 Jun 2018 16:09:26 +0200
Subject: [PATCH] vec_cond_expr_gimple_val
vect_recog_mixed_size_cond_pattern will be disabled
diff --git a/gcc/gimple-loop-interchange.cc b/gcc/gimple-loop-interchange.cc
index eb35263e69c..23fbed590e0 100644
--- a/gcc/gimple-loop-interchange.cc
+++ b/gcc/gimple-loop-interchange.cc
@@ -891,7 +891,9 @@ loop_cand::undo_simple_reduction (reduction_p re, bitmap dce_seeds)
/* Init new_var to MEM_REF or CONST depending on if it is the first
iteration. */
induction_p iv = m_inductions[0];
- cond = fold_build2 (NE_EXPR, boolean_type_node, iv->var, iv->init_val);
+ cond = make_ssa_name (boolean_type_node);
+ stmt = gimple_build_assign (cond, NE_EXPR, iv->var, iv->init_val);
+ gimple_seq_add_stmt_without_update (&stmts, stmt);
new_var = copy_ssa_name (re->var);
stmt = gimple_build_assign (new_var, COND_EXPR, cond, tmp, re->init);
gimple_seq_add_stmt_without_update (&stmts, stmt);
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 4b91151873c..24d7963117e 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -442,6 +442,8 @@ gimple_build_assign_1 (tree lhs, enum tree_code subcode, tree op1,
gimple_build_with_ops_stat (GIMPLE_ASSIGN, (unsigned)subcode, num_ops
PASS_MEM_STAT));
gimple_assign_set_lhs (p, lhs);
+ gcc_assert ((subcode != COND_EXPR && subcode != VEC_COND_EXPR)
+ || is_gimple_val (op1));
gimple_assign_set_rhs1 (p, op1);
if (op2)
{
@@ -1691,6 +1693,8 @@ gimple_assign_set_rhs_with_ops (gimple_stmt_iterator *gsi, enum tree_code code,
gimple_set_num_ops (stmt, new_rhs_ops + 1);
gimple_set_subcode (stmt, code);
+ gcc_assert ((code != COND_EXPR && code != VEC_COND_EXPR)
+ || is_gimple_val (op1));
gimple_assign_set_rhs1 (stmt, op1);
if (new_rhs_ops > 1)
gimple_assign_set_rhs2 (stmt, op2);
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 44cb784620a..b679241e377 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -3899,7 +3899,7 @@ gimplify_pure_cond_expr (tree *expr_p, gimple_seq *pre_p)
TREE_SET_CODE (cond, TRUTH_AND_EXPR);
else if (code == TRUTH_ORIF_EXPR)
TREE_SET_CODE (cond, TRUTH_OR_EXPR);
- ret = gimplify_expr (&cond, pre_p, NULL, is_gimple_condexpr, fb_rvalue);
+ ret = gimplify_expr (&cond, pre_p, NULL, is_gimple_val, fb_rvalue);
COND_EXPR_COND (*expr_p) = cond;
tret = gimplify_expr (&COND_EXPR_THEN (expr), pre_p, NULL,
@@ -12078,7 +12078,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
enum gimplify_status r0, r1, r2;
r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
- post_p, is_gimple_condexpr, fb_rvalue);
+ post_p, is_gimple_val, fb_rvalue);
r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
post_p, is_gimple_val, fb_rvalue);
r2 = gimplify_expr (&TREE_OPERAND (*expr_p, 2), pre_p,
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 21b3fdffa59..5f6defa6fe7 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -4137,10 +4137,7 @@ verify_gimple_assign_ternary (gassign *stmt)
return true;
}
- 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))
+ if (!is_gimple_val (rhs1) || !is_gimple_val (rhs2) || !is_gimple_val (rhs3))
{
error ("invalid operands in ternary operation");
return true;
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 71dac4fb48a..fe9eb83876f 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -1762,6 +1762,9 @@ gen_phi_arg_condition (gphi *phi, vec<int> *occur,
cond = c;
}
gcc_assert (cond != NULL_TREE);
+ cond = force_gimple_operand_gsi_1 (gsi, cond,
+ is_gimple_val, NULL_TREE,
+ true, GSI_SAME_STMT);
return cond;
}
@@ -1844,7 +1847,7 @@ predicate_scalar_phi (gphi *phi, gimple_stmt_iterator *gsi)
cond = bb_predicate (first_edge->src);
/* Gimplify the condition to a valid cond-expr conditonal operand. */
cond = force_gimple_operand_gsi_1 (gsi, unshare_expr (cond),
- is_gimple_condexpr, NULL_TREE,
+ is_gimple_val, NULL_TREE,
true, GSI_SAME_STMT);
true_bb = first_edge->src;
if (EDGE_PRED (bb, 1)->src == true_bb)
@@ -1940,7 +1943,7 @@ predicate_scalar_phi (gphi *phi, gimple_stmt_iterator *gsi)
}
/* Gimplify the condition to a valid cond-expr conditonal operand. */
cond = force_gimple_operand_gsi_1 (gsi, unshare_expr (cond),
- is_gimple_condexpr, NULL_TREE,
+ is_gimple_val, NULL_TREE,
true, GSI_SAME_STMT);
if (!(is_cond_scalar_reduction (phi, &reduc, arg0 , arg1,
&op0, &op1, true)))
@@ -2323,7 +2326,7 @@ predicate_mem_writes (loop_p loop)
if (swap)
std::swap (lhs, rhs);
cond = force_gimple_operand_gsi_1 (&gsi, unshare_expr (cond),
- is_gimple_condexpr, NULL_TREE,
+ is_gimple_val, NULL_TREE,
true, GSI_SAME_STMT);
rhs = fold_build_cond_expr (type, unshare_expr (cond), rhs, lhs);
gimple_assign_set_rhs1 (stmt, ifc_temp_var (type, rhs, &gsi));
diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c
index 2efb5acae8f..95621d1f58b 100644
--- a/gcc/tree-ssa-forwprop.c
+++ b/gcc/tree-ssa-forwprop.c
@@ -504,9 +504,7 @@ forward_propagate_into_comparison (gimple_stmt_iterator *gsi)
/* Propagate from the ssa name definition statements of COND_EXPR
in GIMPLE_COND statement STMT into the conditional if that simplifies it.
Returns zero if no statement was changed, one if there were
- changes and two if cfg_cleanup needs to run.
-
- This must be kept in sync with forward_propagate_into_cond. */
+ changes and two if cfg_cleanup needs to run. */
static int
forward_propagate_into_gimple_cond (gcond *stmt)
@@ -565,70 +563,6 @@ forward_propagate_into_gimple_cond (gcond *stmt)
return 0;
}
-
-/* Propagate from the ssa name definition statements of COND_EXPR
- in the rhs of statement STMT into the conditional if that simplifies it.
- Returns true zero if the stmt was changed. */
-
-static bool
-forward_propagate_into_cond (gimple_stmt_iterator *gsi_p)
-{
- gimple *stmt = gsi_stmt (*gsi_p);
- tree tmp = NULL_TREE;
- tree cond = gimple_assign_rhs1 (stmt);
- enum tree_code code = gimple_assign_rhs_code (stmt);
-
- /* We can do tree combining on SSA_NAME and comparison expressions. */
- if (COMPARISON_CLASS_P (cond))
- tmp = forward_propagate_into_comparison_1 (stmt, TREE_CODE (cond),
- TREE_TYPE (cond),
- TREE_OPERAND (cond, 0),
- TREE_OPERAND (cond, 1));
- else if (TREE_CODE (cond) == SSA_NAME)
- {
- enum tree_code def_code;
- tree name = cond;
- gimple *def_stmt = get_prop_source_stmt (name, true, NULL);
- if (!def_stmt || !can_propagate_from (def_stmt))
- return 0;
-
- def_code = gimple_assign_rhs_code (def_stmt);
- if (TREE_CODE_CLASS (def_code) == tcc_comparison)
- tmp = fold_build2_loc (gimple_location (def_stmt),
- def_code,
- TREE_TYPE (cond),
- gimple_assign_rhs1 (def_stmt),
- gimple_assign_rhs2 (def_stmt));
- }
-
- if (tmp
- && is_gimple_condexpr (tmp))
- {
- if (dump_file && tmp)
- {
- fprintf (dump_file, " Replaced '");
- print_generic_expr (dump_file, cond);
- fprintf (dump_file, "' with '");
- print_generic_expr (dump_file, tmp);
- fprintf (dump_file, "'\n");
- }
-
- if ((code == VEC_COND_EXPR) ? integer_all_onesp (tmp)
- : integer_onep (tmp))
- gimple_assign_set_rhs_from_tree (gsi_p, gimple_assign_rhs2 (stmt));
- else if (integer_zerop (tmp))
- gimple_assign_set_rhs_from_tree (gsi_p, gimple_assign_rhs3 (stmt));
- else
- gimple_assign_set_rhs1 (stmt, unshare_expr (tmp));
- stmt = gsi_stmt (*gsi_p);
- update_stmt (stmt);
-
- return true;
- }
-
- return 0;
-}
-
/* We've just substituted an ADDR_EXPR into stmt. Update all the
relevant data structures to match. */
@@ -2482,17 +2416,7 @@ pass_forwprop::execute (function *fun)
tree rhs1 = gimple_assign_rhs1 (stmt);
enum tree_code code = gimple_assign_rhs_code (stmt);
- if (code == COND_EXPR
- || code == VEC_COND_EXPR)
- {
- /* In this case the entire COND_EXPR is in rhs1. */
- if (forward_propagate_into_cond (&gsi))
- {
- changed = true;
- stmt = gsi_stmt (gsi);
- }
- }
- else if (TREE_CODE_CLASS (code) == tcc_comparison)
+ if (TREE_CODE_CLASS (code) == tcc_comparison)
{
int did_something;
did_something = forward_propagate_into_comparison (&gsi);
diff --git a/gcc/tree-ssa-loop-im.c b/gcc/tree-ssa-loop-im.c
index 01a954eeb1e..df010e1087f 100644
--- a/gcc/tree-ssa-loop-im.c
+++ b/gcc/tree-ssa-loop-im.c
@@ -1140,8 +1140,11 @@ move_computations_worker (basic_block bb)
edges of COND. */
extract_true_false_args_from_phi (dom, stmt, &arg0, &arg1);
gcc_assert (arg0 && arg1);
- t = build2 (gimple_cond_code (cond), boolean_type_node,
- gimple_cond_lhs (cond), gimple_cond_rhs (cond));
+ t = make_ssa_name (boolean_type_node);
+ new_stmt = gimple_build_assign (t, gimple_cond_code (cond),
+ gimple_cond_lhs (cond),
+ gimple_cond_rhs (cond));
+ gsi_insert_on_edge (loop_preheader_edge (level), new_stmt);
new_stmt = gimple_build_assign (gimple_phi_result (stmt),
COND_EXPR, t, arg0, arg1);
todo |= TODO_cleanup_cfg;
diff --git a/gcc/tree-vect-generic.c b/gcc/tree-vect-generic.c
index 46502c42c74..66c6fe2237a 100644
--- a/gcc/tree-vect-generic.c
+++ b/gcc/tree-vect-generic.c
@@ -655,7 +655,9 @@ expand_vector_divmod (gimple_stmt_iterator *gsi, tree type, tree op0,
mask_type = build_same_sized_truth_vector_type (type);
zero = build_zero_cst (type);
- cond = build2 (LT_EXPR, mask_type, op0, zero);
+ cond = make_ssa_name (mask_type);
+ stmt = gimple_build_assign (cond, LT_EXPR, op0, zero);
+ gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
tree_vector_builder vec (type, nunits, 1);
for (i = 0; i < nunits; i++)
vec.quick_push (build_int_cst (TREE_TYPE (type),
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index f99484f1da5..507c5b94f07 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -2701,8 +2701,10 @@ vect_recog_divmod_pattern (vec<gimple *> *stmts,
dump_printf_loc (MSG_NOTE, vect_location,
"vect_recog_divmod_pattern: detected:\n");
- cond = build2 (LT_EXPR, boolean_type_node, oprnd0,
- build_int_cst (itype, 0));
+ cond = make_ssa_name (boolean_type_node);
+ def_stmt = gimple_build_assign (cond, LT_EXPR, oprnd0,
+ build_int_cst (itype, 0));
+ new_pattern_def_seq (stmt_vinfo, def_stmt);
if (rhs_code == TRUNC_DIV_EXPR)
{
tree var = vect_recog_temp_ssa_var (itype, NULL);
@@ -2712,7 +2714,7 @@ vect_recog_divmod_pattern (vec<gimple *> *stmts,
fold_build2 (MINUS_EXPR, itype, oprnd1,
build_int_cst (itype, 1)),
build_int_cst (itype, 0));
- new_pattern_def_seq (stmt_vinfo, def_stmt);
+ append_pattern_def_seq (stmt_vinfo, def_stmt);
var = vect_recog_temp_ssa_var (itype, NULL);
def_stmt
= gimple_build_assign (var, PLUS_EXPR, oprnd0,
@@ -2727,7 +2729,6 @@ vect_recog_divmod_pattern (vec<gimple *> *stmts,
else
{
tree signmask;
- STMT_VINFO_PATTERN_DEF_SEQ (stmt_vinfo) = NULL;
if (compare_tree_int (oprnd1, 2) == 0)
{
signmask = vect_recog_temp_ssa_var (itype, NULL);
@@ -3483,7 +3484,9 @@ adjust_bool_pattern (tree var, tree out_type,
}
else
itype = TREE_TYPE (rhs1);
- cond_expr = build2_loc (loc, rhs_code, itype, rhs1, rhs2);
+ cond_expr = make_ssa_name (itype);
+ pattern_stmt = gimple_build_assign (cond_expr, rhs_code, rhs1, rhs2);
+ append_pattern_def_seq (stmt_info, pattern_stmt);
if (trueval == NULL_TREE)
trueval = build_int_cst (itype, 1);
else
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 74f813e7334..9956b453fd5 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -4160,7 +4160,9 @@ vr_values::simplify_stmt_using_ranges (gimple_stmt_iterator *gsi)
in divide by zero, new_rhs1 / new_rhs will be NULL_TREE. */
if (new_rhs1 && new_rhs2)
{
- tree cond = build2 (EQ_EXPR, boolean_type_node, cmp_var, val1);
+ tree cond = make_ssa_name (boolean_type_node);
+ gimple *def = gimple_build_assign (cond, EQ_EXPR, cmp_var, val1);
+ gsi_insert_before (gsi, def, GSI_SAME_STMT);
gimple_assign_set_rhs_with_ops (gsi,
COND_EXPR, cond,
new_rhs1,