Re: [PATCH] Correct cost model for strided loads

[Richard Guenther <rguenther@suse.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6718 bytes --]

On Mon, 11 Jun 2012, William J. Schmidt wrote:

> 
> 
> On Mon, 2012-06-11 at 11:15 +0200, Richard Guenther wrote:
> > On Sun, Jun 10, 2012 at 5:58 PM, William J. Schmidt
> > <wschmidt@linux.vnet.ibm.com> wrote:
> > > The fix for PR53331 caused a degradation to 187.facerec on
> > > powerpc64-unknown-linux-gnu.  The following simple patch reverses the
> > > degradation without otherwise affecting SPEC cpu2000 or cpu2006.
> > > Bootstrapped and regtested on that platform with no new regressions.  Ok
> > > for trunk?
> > 
> > Well, would the real cost not be subparts * scalar_to_vec plus
> > subparts * vec_perm?
> > At least vec_perm isn't the cost for building up a vector from N scalar elements
> > either (it might be close enough if subparts == 2).  What's the case
> > with facerec
> > here?  Does it have subparts == 2?  
> 
> In this case, subparts == 4 (32-bit floats, 128-bit vec reg).  On
> PowerPC, this requires two merge instructions and a permute instruction
> to get the four 32-bit quantities into the right place in a 128-bit
> register.  Currently this is modeled as a vec_perm in other parts of the
> vectorizer per Ira's earlier patches, so I naively changed this to do
> the same thing.

I see.

> The types of vectorizer instructions aren't documented, and I can't
> infer much from the i386.c cost model, so I need a little education.
> What semantics are represented by scalar_to_vec?

It's a vector splat, thus x -> { x, x, x, ... }.  You can create
{ x, y, z, ... } by N such splats plus N - 1 permutes (if a permute,
as VEC_PERM_EXPR, takes two input vectors).  That's by far not
the most efficient way to build up such a vector of course (with AVX
you could do one splat plus N - 1 inserts for example).  The cost
is of course dependent on the number of vector elements, so a
simple new enum vect_cost_for_stmt kind does not cover it but
the target would have to look at the vector type passed and do
some reasonable guess.

> On PowerPC, we have this mapping of the floating-point registers and
> vector float registers where they overlap (the low-order half of each of
> the first 32 vector float regs corresponds to a scalar float reg).  So
> in this case we have four scalar loads that place things in the bottom
> half of four vector registers, two vector merge instructions that
> collapse the four registers into two vector registers, and a vector
> permute that gets things in the right order.(*)  I wonder if what we
> refer to as a merge instruction is similar to scalar_to_vec.

Looks similar to x86 SSE then.

> If so, then maybe we need something like
> 
>      subparts = TYPE_VECTOR_SUBPARTS (STMT_VINFO_VECTYPE (stmt_info));
>      inside_cost += vect_get_stmt_cost (scalar_load) * ncopies * subparts;
>      inside_cost += ncopies * vect_get_stmt_cost (scalar_to_vec) * subparts / 2;
>      inside_cost += ncopies * vect_get_stmt_cost (vec_perm);
> 
> But then we'd have to change how vec_perm is modeled elsewhere for
> PowerPC based on Ira's earlier patches.  As I said, it's difficult for
> me to figure out all the intent of cost model decisions that have been
> made in the past, using current documentation.

Heh, usually the intent was to make the changes simple, not to compute
a proper cost.

I think we simply need a new scalars_to_vec cost kind.

> > I really wanted to pessimize this case
> > for say AVX and char elements, thus building up a vector from 32 scalars which
> > certainly does not cost a mere vec_perm.  So, maybe special-case the
> > subparts == 2 case and assume vec_perm would match the cost only in that
> > case.
> 
> (I'm a little confused by this as what you have at the moment is a
> single scalar_to_vec per copy, which has a cost of 1 on most i386
> targets (occasionally 2).  The subparts multiplier is only applied to
> the loads.  So changing this to vec_perm seemed to be a no-op for i386.)

Oh, I somehow read the patch as you were removing the multiplication
by TYPE_VECTOR_SUBPARTS.  But yes, the cost is way off and I'd wanted
to reflect it with N scalar loads plus N splats plus N - 1 permutes
originally.  You could also model it with N scalar loads plus N
inserts (but we don't have a vec_insert cost either).  I think adding
a scalars_to_vec or vec_init or however we want to call it - basically
what the cost of a vector CONSTRUCTOR would be - is best.

> (*) There are actually a couple more instructions here to convert 64-bit
> values to 32-bit values, since on PowerPC 32-bit loads are converted to
> 64-bit values in scalar float registers and they have to be coerced back
> to 32-bit.  Very ugly.  The cost model currently doesn't represent this
> at all, which I'll have to look at fixing at some point in some way that
> isn't too nasty for the other targets.  The cost model for PowerPC seems
> to need a lot of TLC.

Maybe the above would be a possible way to do it.  On x86 for example
a vector of two doubles is extremely cheap for generic SSE2 to construct,
we can directly load into the low/high part, thus when we use it as
N scalar loads plus the vector-build the vector-build would be free.

Thanks,
Richard.

> Thanks,
> Bill
> 
> > 
> > Thanks,
> > Richard.
> > 
> > > Thanks,
> > > Bill
> > >
> > >
> > > 2012-06-10  Bill Schmidt  <wschmidt@linux.ibm.com>
> > >
> > >        * tree-vect-stmts.c (vect_model_load_cost):  Change cost model
> > >        for strided loads.
> > >
> > >
> > > Index: gcc/tree-vect-stmts.c
> > > ===================================================================
> > > --- gcc/tree-vect-stmts.c       (revision 188341)
> > > +++ gcc/tree-vect-stmts.c       (working copy)
> > > @@ -1031,11 +1031,10 @@ vect_model_load_cost (stmt_vec_info stmt_info, int
> > >   /* The loads themselves.  */
> > >   if (STMT_VINFO_STRIDE_LOAD_P (stmt_info))
> > >     {
> > > -      /* N scalar loads plus gathering them into a vector.
> > > -         ???  scalar_to_vec isn't the cost for that.  */
> > > +      /* N scalar loads plus gathering them into a vector.  */
> > >       inside_cost += (vect_get_stmt_cost (scalar_load) * ncopies
> > >                      * TYPE_VECTOR_SUBPARTS (STMT_VINFO_VECTYPE (stmt_info)));
> > > -      inside_cost += ncopies * vect_get_stmt_cost (scalar_to_vec);
> > > +      inside_cost += ncopies * vect_get_stmt_cost (vec_perm);
> > >     }
> > >   else
> > >     vect_get_load_cost (first_dr, ncopies,
> > >
> > >
> > 
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer