[PATCH v4] rs6000: Add load density heuristic

[Segher Boessenkool]

Hi!

On Thu, Sep 09, 2021 at 12:19:28PM -0500, Bill Schmidt wrote:
> On 9/9/21 11:11 AM, Segher Boessenkool wrote:
> >On Wed, Sep 08, 2021 at 02:57:14PM +0800, Kewen.Lin wrote:
> >>+      /* If we have strided or elementwise loads into a vector, it's
> >"strided" is not a word: it properly is "stridden", which does not read
> >very well either.  "Have loads by stride, or by element, ..."?  Is that
> >good English, and easier to understand?
> 
> No, this is OK.  "Strided loads" is a term of art used by the 
> vectorizer; whether or not it was the Queen's English, it's what we 
> have...

"Strided" is not in any dictionary I could find.  I have no idea what
the island queen has to do with anything :-)

> (And I think you might only find "bestridden" in some 18th or 
> 19th century English poetry... :-)

"Bestride" is not used in the US apparently, but it has nothing to do
with this anyway.

In any case, "strided" is used a lot in the vectoriser already (and
nowhere else).  So, fine with me of course.

> >Does that text look good to you now Bill?  It is still kinda complex,
> >maybe you see a way to make it simpler.
> 
> I think it's OK now.  The complexity at least matches the code now 
> instead of exceeding it. :-P  j/k...

Ha :-)

Ideally the comments will clarify complex code.  But not making it worse
is at least something, okay :-)

> >>+  if (data->extra_ctor_cost > 0)
> >>+    {
> >>+      /* Threshold for load stmts percentage in all vectorized stmts.  */
> >>+      const int DENSITY_LOAD_PCT_THRESHOLD = 45;
> >Threshold for what?
> >
> >45% is awfully exact.  Can you make this a param?
> >
> >>+      /* Threshold for total number of load stmts.  */
> >>+      const int DENSITY_LOAD_NUM_THRESHOLD = 20;
> >Same.
> 
> We have similar magic constants in here already.

And that is problematic.  That is the problem.

> Parameterizing is 
> possible, but I'm more interested in making sure the numbers are 
> appropriate for each processor.

Of course.  So a) there shouldn't be any magic constants, the constants
should be meaningful.  That way, tunings will need less maintenance for
new hardware versions, but way more importantly: if anything else in the
compiler changes.  And b), having params makes it easier to do such
tuning, or experiments for it.

It is very easy to add params btw (just in rs6000.opt).

> Perhaps a follow-up patch to add params for the magic constants would be 
> reasonable, but I'd personally consider it pretty low priority.

It also is super easy to do.  The only hard part is making a name for
it, and if you cannot think of a good name, that says that it is not a
good abstraction in the first place -- magic isn't good.

Please do it.

> >>+	  /* As function rs6000_builtin_vectorization_cost shows, we have
> >>+	     priced much on V16QI/V8HI vector construction as their units,
> >>+	     if we penalize them with nunits * stmt_cost, it can result in
> >>+	     an unreliable body cost, eg: for V16QI on Power8, stmt_cost
> >>+	     is 20 and nunits is 16, the extra cost is 320 which looks
> >>+	     much exaggerated.  So let's use one maximum bound for the
> >>+	     extra penalized cost for vector construction here.  */
> >>+	  const unsigned int MAX_PENALIZED_COST_FOR_CTOR = 12;
> >>+	  if (extra_cost > MAX_PENALIZED_COST_FOR_CTOR)
> >>+	    extra_cost = MAX_PENALIZED_COST_FOR_CTOR;
> >That is a pretty gross hack.  Can you think of any saner way to not have
> >those out of scale costs in the first place?
> 
> In Kewen's defense, the whole business of "finish_cost" for these 
> vectorized loops is to tweak things that don't work quite right with the 
> hooks currently provided to the vectorizer to add costs on a per-stmt 
> basis without looking at the overall set of statements.  It gives the 
> back end a chance to massage things and exercise veto power over 
> otherwise bad decisions.  By nature, that's going to be very much a 
> heuristic exercise.  Personally I think the heuristics used here are 
> pretty reasonable, and importantly they are designed to only be employed 
> in pretty rare circumstances.  It doesn't look easy to me to avoid the 
> need for a cap here without making the rest of the heuristics harder to 
> understand.  But sure, he can try! :)

Yes.  But we need to avoid magic.  Magic does not scale; magic makes it
hard or impossible to do any future changes.

I did approve the patch already.  But weaknesses like this need
continuous attention, and that does not scale.


Segher