Re: [PATCH 3/3] target/99881 - x86 vector cost of CTOR from integer regs

[Hongtao Liu <crazylht@gmail.com>]

On Mon, Feb 21, 2022 at 5:10 PM Richard Biener <rguenther@suse.de> wrote:
>
> On Mon, 21 Feb 2022, Hongtao Liu wrote:
>
> > On Fri, Feb 18, 2022 at 10:01 PM Richard Biener via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > This uses the now passed SLP node to the vectorizer costing hook
> > > to adjust vector construction costs for the cost of moving an
> > > integer component from a GPR to a vector register when that's
> > > required for building a vector from components.  A cruical difference
> > > here is whether the component is loaded from memory or extracted
> > > from a vector register as in those cases no intermediate GPR is involved.
> > >
> > > The pr99881.c testcase can be Un-XFAILed with this patch, the
> > > pr91446.c testcase now produces scalar code which looks superior
> > > to me so I've adjusted it as well.
> > >
> > > I'm currently re-bootstrapping and testing on x86_64-unknown-linux-gnu
> > > after adding the BIT_FIELD_REF vector extracting special casing.
> > Does the patch handle PR101929?
>
> The patch will regress the testcase posted in PR101929 again:
>
>  _255 1 times scalar_store costs 12 in body
>  _261 1 times scalar_store costs 12 in body
>  _258 1 times scalar_store costs 12 in body
>  _264 1 times scalar_store costs 12 in body
>  t0_247 + t2_251 1 times scalar_stmt costs 4 in body
>  t1_472 + t3_444 1 times scalar_stmt costs 4 in body
>  t0_406 - t2_451 1 times scalar_stmt costs 4 in body
>  t1_472 - t3_444 1 times scalar_stmt costs 4 in body
> -node 0x4182f48 1 times vec_construct costs 16 in prologue
> -node 0x41882b0 1 times vec_construct costs 16 in prologue
> +node 0x4182f48 1 times vec_construct costs 28 in prologue
> +node 0x41882b0 1 times vec_construct costs 28 in prologue
>  t0_406 + t2_451 1 times vector_stmt costs 4 in body
>  t1_472 - t3_444 1 times vector_stmt costs 4 in body
>  node 0x41829f8 1 times vec_perm costs 4 in body
>  _436 1 times vector_store costs 16 in body
>  t.c:37:9: note: Cost model analysis for part in loop 0:
> -  Vector cost: 60
> +  Vector cost: 84
>    Scalar cost: 64
> +t.c:37:9: missed: not vectorized: vectorization is not profitable.
>
> We're constructing V4SI from patterns like { _407, _480, _407, _480 }
> where the components are results of integer adds (so the result is
> definitely in a GPR).  We are costing the construction as
> 4 * sse_op + 2 * sse_to_integer which with skylake cost is
> 4 * COSTS_N_INSNS (1) + 2 * 6.
>
> Whether the vectorization itself is profitable is likely questionable
> but then it's true that the construction of V4SI is more costly
> in terms of uops than a construction of V4SF.
>
> Now, we can - for the first time - now see the actual construction
> pattern and ideal construction might be two GPR->xmm moves
> + two splats + one unpack or maybe two GPR->xmm moves + one
> unpack + splat of DI (or other means of duplicating the lowpart).
Yes, the patch is technically right. I'm ok with the patch.
> It still wouldn't help here of course, and we would not have RTL
> expansion adhere to this scheme.
>
> Trying to capture secondary effects (the FRE opportunity unleashed)
> in costing of this particular SLP subgraph is difficult and probably
> undesirable though.  Adjusting any of the target specific costs
> is likely not going to work because of the original narrow profitability
> gap (60 vs. 64).
>
> For the particular kernel the overall vectorization strathegy needs
> to improve (PR98138 is about this I think).  I know we've reverted
> the previous change that attempted to cost for GPR -> XMM.  It did
>
>        case vec_construct:
>         {
>           /* N element inserts into SSE vectors.  */
> +         int cost
> +           = TYPE_VECTOR_SUBPARTS (vectype) * (fp ?
> +                                               ix86_cost->sse_op
> +                                               :
> ix86_cost->integer_to_sse);
> +
> -         int cost = TYPE_VECTOR_SUBPARTS (vectype) * ix86_cost->sse_op;
>
> thus treated integer_to_sse as GPR insert into vector cost for
> integer components in general while the now proposed change
> _adds_ integer to XMM move costs (but only once for duplicate
> elements and not for the cases we know are from memory / vector regs).
> With the proposed patch we can probably be more "correct" above
> and only cost TYPE_VECTOR_SUBPARTS (vectype) - 1 inserts given
> for FP the first one is free and for int we're costing it separately.
> Again it won't help here.
>
> Thanks,
> Richard.
>
> > >
> > > I suppose we can let autotesters look for SPEC performance fallout.
> > >
> > > OK if testing succeeds?
> > >
> > > Thanks,
> > > Richard.
> > >
> > > 2022-02-18  Richard Biener  <rguenther@suse.de>
> > >
> > >         PR tree-optimization/104582
> > >         PR target/99881
> > >         * config/i386/i386.cc (ix86_vector_costs::add_stmt_cost):
> > >         Cost GPR to vector register moves for integer vector construction.
> > >
> > >         * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c: New.
> > >         * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c: Likewise.
> > >         * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c: Likewise.
> > >         * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c: Likewise.
> > >         * gcc.target/i386/pr99881.c: Un-XFAIL.
> > >         * gcc.target/i386/pr91446.c: Adjust to not expect vectorization.
> > > ---
> > >  gcc/config/i386/i386.cc                       | 45 ++++++++++++++++++-
> > >  .../costmodel/x86_64/costmodel-pr104582-1.c   | 15 +++++++
> > >  .../costmodel/x86_64/costmodel-pr104582-2.c   | 13 ++++++
> > >  .../costmodel/x86_64/costmodel-pr104582-3.c   | 13 ++++++
> > >  .../costmodel/x86_64/costmodel-pr104582-4.c   | 15 +++++++
> > >  gcc/testsuite/gcc.target/i386/pr91446.c       |  2 +-
> > >  gcc/testsuite/gcc.target/i386/pr99881.c       |  2 +-
> > >  7 files changed, 102 insertions(+), 3 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c
> > >
> > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> > > index 0830dbd7dca..b2bf90576d5 100644
> > > --- a/gcc/config/i386/i386.cc
> > > +++ b/gcc/config/i386/i386.cc
> > > @@ -22997,7 +22997,7 @@ ix86_vectorize_create_costs (vec_info *vinfo, bool costing_for_scalar)
> > >
> > >  unsigned
> > >  ix86_vector_costs::add_stmt_cost (int count, vect_cost_for_stmt kind,
> > > -                                 stmt_vec_info stmt_info, slp_tree,
> > > +                                 stmt_vec_info stmt_info, slp_tree node,
> > >                                   tree vectype, int misalign,
> > >                                   vect_cost_model_location where)
> > >  {
> > > @@ -23160,6 +23160,49 @@ ix86_vector_costs::add_stmt_cost (int count, vect_cost_for_stmt kind,
> > >        stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign);
> > >        stmt_cost *= (TYPE_VECTOR_SUBPARTS (vectype) + 1);
> > >      }
> > > +  else if (kind == vec_construct
> > > +          && node
> > > +          && SLP_TREE_DEF_TYPE (node) == vect_external_def
> > > +          && INTEGRAL_TYPE_P (TREE_TYPE (vectype)))
> > > +    {
> > > +      stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign);
> > > +      unsigned i;
> > > +      tree op;
> > > +      FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_OPS (node), i, op)
> > > +       if (TREE_CODE (op) == SSA_NAME)
> > > +         TREE_VISITED (op) = 0;
> > > +      FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_OPS (node), i, op)
> > > +       {
> > > +         if (TREE_CODE (op) != SSA_NAME
> > > +             || TREE_VISITED (op))
> > > +           continue;
> > > +         TREE_VISITED (op) = 1;
> > > +         gimple *def = SSA_NAME_DEF_STMT (op);
> > > +         tree tem;
> > > +         if (is_gimple_assign (def)
> > > +             && CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def))
> > > +             && ((tem = gimple_assign_rhs1 (def)), true)
> > > +             && TREE_CODE (tem) == SSA_NAME
> > > +             /* A sign-change expands to nothing.  */
> > > +             && tree_nop_conversion_p (TREE_TYPE (gimple_assign_lhs (def)),
> > > +                                       TREE_TYPE (tem)))
> > > +           def = SSA_NAME_DEF_STMT (tem);
> > > +         /* When the component is loaded from memory we can directly
> > > +            move it to a vector register, otherwise we have to go
> > > +            via a GPR or via vpinsr which involves similar cost.
> > > +            Likewise with a BIT_FIELD_REF extracting from a vector
> > > +            register we can hope to avoid using a GPR.  */
> > > +         if (!is_gimple_assign (def)
> > > +             || (!gimple_assign_load_p (def)
> > > +                 && (gimple_assign_rhs_code (def) != BIT_FIELD_REF
> > > +                     || !VECTOR_TYPE_P (TREE_TYPE
> > > +                               (TREE_OPERAND (gimple_assign_rhs1 (def), 0))))))
> > > +           stmt_cost += ix86_cost->sse_to_integer;
> > > +       }
> > > +      FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_OPS (node), i, op)
> > > +       if (TREE_CODE (op) == SSA_NAME)
> > > +         TREE_VISITED (op) = 0;
> > > +    }
> > >    if (stmt_cost == -1)
> > >      stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign);
> > >
> > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c
> > > new file mode 100644
> > > index 00000000000..992a845ad7a
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c
> > > @@ -0,0 +1,15 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */
> > > +
> > > +struct S { unsigned long a, b; } s;
> > > +
> > > +void
> > > +foo (unsigned long *a, unsigned long *b)
> > > +{
> > > +  unsigned long a_ = *a;
> > > +  unsigned long b_ = *b;
> > > +  s.a = a_;
> > > +  s.b = b_;
> > > +}
> > > +
> > > +/* { dg-final { scan-tree-dump "basic block part vectorized" "slp2" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c
> > > new file mode 100644
> > > index 00000000000..7637cdb4a97
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c
> > > @@ -0,0 +1,13 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */
> > > +
> > > +struct S { unsigned long a, b; } s;
> > > +
> > > +void
> > > +foo (unsigned long a, unsigned long b)
> > > +{
> > > +  s.a = a;
> > > +  s.b = b;
> > > +}
> > > +
> > > +/* { dg-final { scan-tree-dump-not "basic block part vectorized" "slp2" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c
> > > new file mode 100644
> > > index 00000000000..999c4905708
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c
> > > @@ -0,0 +1,13 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */
> > > +
> > > +struct S { double a, b; } s;
> > > +
> > > +void
> > > +foo (double a, double b)
> > > +{
> > > +  s.a = a;
> > > +  s.b = b;
> > > +}
> > > +
> > > +/* { dg-final { scan-tree-dump "basic block part vectorized" "slp2" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c
> > > new file mode 100644
> > > index 00000000000..cc471e1ed73
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c
> > > @@ -0,0 +1,15 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */
> > > +
> > > +struct S { unsigned long a, b; } s;
> > > +
> > > +void
> > > +foo (signed long *a, unsigned long *b)
> > > +{
> > > +  unsigned long a_ = *a;
> > > +  unsigned long b_ = *b;
> > > +  s.a = a_;
> > > +  s.b = b_;
> > > +}
> > > +
> > > +/* { dg-final { scan-tree-dump "basic block part vectorized" "slp2" } } */
> > > diff --git a/gcc/testsuite/gcc.target/i386/pr91446.c b/gcc/testsuite/gcc.target/i386/pr91446.c
> > > index 0243ca3ea68..067bf43f698 100644
> > > --- a/gcc/testsuite/gcc.target/i386/pr91446.c
> > > +++ b/gcc/testsuite/gcc.target/i386/pr91446.c
> > > @@ -21,4 +21,4 @@ foo (unsigned long long width, unsigned long long height,
> > >    bar (&t);
> > >  }
> > >
> > > -/* { dg-final { scan-assembler-times "vmovdqa\[^\n\r\]*xmm\[0-9\]" 2 } } */
> > > +/* { dg-final { scan-assembler-times "xmm\[0-9\]" 0 } } */
> > > diff --git a/gcc/testsuite/gcc.target/i386/pr99881.c b/gcc/testsuite/gcc.target/i386/pr99881.c
> > > index 3e087eb2ed7..a1ec1d1ba8a 100644
> > > --- a/gcc/testsuite/gcc.target/i386/pr99881.c
> > > +++ b/gcc/testsuite/gcc.target/i386/pr99881.c
> > > @@ -1,7 +1,7 @@
> > >  /* PR target/99881.  */
> > >  /* { dg-do compile { target { ! ia32 } } } */
> > >  /* { dg-options "-Ofast -march=skylake" } */
> > > -/* { dg-final { scan-assembler-not "xmm\[0-9\]" { xfail *-*-* } } } */
> > > +/* { dg-final { scan-assembler-not "xmm\[0-9\]" } } */
> > >
> > >  void
> > >  foo (int* __restrict a, int n, int c)
> > > --
> > > 2.34.1
> >
> >
> >
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)



-- 
BR,
Hongtao