On Tue, Jun 30, 2020 at 2:16 PM Martin Liška <mli...@suse.cz> wrote: > > On 6/30/20 12:38 PM, Richard Biener wrote: > > On Tue, Jun 30, 2020 at 11:44 AM Martin Liška <mli...@suse.cz> wrote: > >> > >> Hi. > >> > >> The patch is about blocking of vector expansion of comparisons > >> that are only feeding a VEC_COND_EXPR statements. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> The problematic mips64 test-case looks good now. > >> > >> Ready to be installed? > > > > So why does > > > > static tree > > expand_vector_comparison (gimple_stmt_iterator *gsi, tree type, tree op0, > > tree op1, enum tree_code code) > > { > > tree t; > > if (!expand_vec_cmp_expr_p (TREE_TYPE (op0), type, code) > > && !expand_vec_cond_expr_p (type, TREE_TYPE (op0), code)) > > ^^^^ > > > > not return true > > Apparently because it's called for type: > (gdb) p debug_tree(gimple_expr_type(stmt)) > <vector_type 0x7ffff7721348 > type <boolean_type 0x7ffff77212a0 SI > size <integer_cst 0x7ffff76338e8 constant 32> > unit-size <integer_cst 0x7ffff7633900 constant 4> > align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > 0x7ffff77212a0 precision:32 min <integer_cst 0x7ffff773e750 -2147483648> max > <integer_cst 0x7ffff773e768 2147483647>> > BLK > size <integer_cst 0x7ffff76336a8 type <integer_type 0x7ffff76370a8 > bitsizetype> constant 64> > unit-size <integer_cst 0x7ffff76336c0 type <integer_type 0x7ffff7637000 > sizetype> constant 8> > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > 0x7ffff7721348 nunits:2> > > while ... > > > { > > > > but > > > > /* Expand a vector condition to scalars, by using many conditions > > on the vector's elements. */ > > static void > > expand_vector_condition (gimple_stmt_iterator *gsi, auto_bitmap > > *dce_ssa_names) > > { > > ... > > if (expand_vec_cond_expr_p (type, TREE_TYPE (a1), code)) > > { > > gcc_assert (TREE_CODE (a) == SSA_NAME || TREE_CODE (a) == > > VECTOR_CST); > > return; > > > > does? > > this has > > <vector_type 0x7ffff76fb3f0 > type <real_type 0x7ffff763e2a0 float SF > size <integer_cst 0x7ffff76338e8 constant 32> > unit-size <integer_cst 0x7ffff7633900 constant 4> > align:32 warn_if_not_align:0 symtab:0 alias-set 1 canonical-type > 0x7ffff763e2a0 precision:32 > pointer_to_this <pointer_type 0x7ffff763e7e0>> > V2SF > size <integer_cst 0x7ffff76336a8 type <integer_type 0x7ffff76370a8 > bitsizetype> constant 64> > unit-size <integer_cst 0x7ffff76336c0 type <integer_type 0x7ffff7637000 > sizetype> constant 8> > align:64 warn_if_not_align:0 symtab:0 alias-set 1 canonical-type > 0x7ffff76fb3f0 nunits:2 > pointer_to_this <pointer_type 0x7ffff7721000>> > > > What's special about the problematical mips testcase? > > That we end up with something like: > > _34 = {_29, _37}; > vect_iftmp.12_35 = VEC_COND_EXPR <_34, vect__1.7_28, vect__2.10_31>; > > which can't be expanded to .VCOND, but we expand to series of BIT_FIELD_REFs: > > _40 = BIT_FIELD_REF <vect__2.10_31, 32, 0>; > _22 = BIT_FIELD_REF <_34, 32, 0>; > _10 = _22 != 0 ? _26 : _40; > _16 = BIT_FIELD_REF <vect__2.10_31, 32, 32>; > _18 = BIT_FIELD_REF <_34, 32, 32>; > _41 = _18 == 0 ? _16 : _30; > > > > Can't you produce > > the same "bad" result when the comparison is used in a non-VEC_COND? > > Theoretically yes. > > Anyway, question is how much do we care about the fallout as it happens for > MIPS and > it's only a worse code stuff?
We certainly care about the fact that vector lowering will lower the condition first and only then come along a VEC_COND_EXPR and lowers that as well. It's classical fallout of separating the compare from the condition. I see vector lowering works on BBs in "random" order and I've always thought that it should work more like complex lowering using a lattice to be able to optimize things better. Now to simply restore previous behavior for this particular case we should probably make expand_vector_comparison walk immediate uses as you do but then call expand_vector_condition for each VEC_COND_EXPR use, making that return whether it "consumed" the comparison. If all uses consumed the comparison we should DCE it, if not, we should lower it? Richard. > Martin > > > > > There's a PR reference missing in the ChangeLog. > > > > Richard. > > > >> Thanks, > >> Martin > >> > >> gcc/ChangeLog: > >> > >> * tree-vect-generic.c (expand_vector_comparison): Do not expand > >> comparison that only feed first argument of a VEC_COND_EXPR > >> statement. > >> --- > >> gcc/tree-vect-generic.c | 24 ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/gcc/tree-vect-generic.c b/gcc/tree-vect-generic.c > >> index a4b56195903..4606decd0f2 100644 > >> --- a/gcc/tree-vect-generic.c > >> +++ b/gcc/tree-vect-generic.c > >> @@ -379,6 +379,30 @@ static tree > >> expand_vector_comparison (gimple_stmt_iterator *gsi, tree type, tree > >> op0, > >> tree op1, enum tree_code code) > >> { > >> + tree lhs = gimple_assign_lhs (gsi_stmt (*gsi)); > >> + use_operand_p use_p; > >> + imm_use_iterator iterator; > >> + bool vec_cond_expr_only = true; > >> + bool has_use = false; > >> + > >> + /* As seen in PR95830, we should not expand comparisons that are only > >> + feeding a VEC_COND_EXPR statement. */ > >> + FOR_EACH_IMM_USE_FAST (use_p, iterator, lhs) > >> + { > >> + has_use = true; > >> + gassign *use = dyn_cast<gassign *> (USE_STMT (use_p)); > >> + if (use == NULL > >> + || gimple_assign_rhs_code (use) != VEC_COND_EXPR > >> + || gimple_assign_rhs1 (use) != lhs) > >> + { > >> + vec_cond_expr_only = false; > >> + break; > >> + } > >> + } > >> + > >> + if (has_use && vec_cond_expr_only) > >> + return NULL_TREE; > >> + > >> tree t; > >> if (!expand_vec_cmp_expr_p (TREE_TYPE (op0), type, code) > >> && !expand_vec_cond_expr_p (type, TREE_TYPE (op0), code)) > >> -- > >> 2.27.0 > >> >