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
> >>
>

Reply via email to