Gentle ping. Any further opinions on this?

Cheers, LGTM

It's strange that I first increase the cost, and this proposes it to lower it. Clearly we need some alignment and clarity on this. But most importantly, we need some motivating examples before we continue with any of this.

I did some performance runs, nothing stands out, so LGTM.

Sorry, I also forgot about this, but thanks for pinging me.

In D109958#4206644, @nikic wrote:

FWIW, I am quite unhappy with the implementation quality of this pass, but I don't think I have the energy to deal with this. In the future, due diligence for pass enablement needs to include a review of the pass implementation by a domain expert, if this was not already done as part of the initial implementation. (Domain expert = SCEV reviewer in this context.)

Cheers, this makes a lot of sense to me.

Just a heads up that I am going to recommit this tomorrow.

Added FNEG, and checks for BF16 and FP16.

In D146033#4202014, @dmgreen wrote:

I think this makes a lot of sense. Especially if we are treating many shuffles with a cost of 1, floating point operations should not be twice the cost. We could consider doing the same for fmul from looking at software optimization guides, but the changes for fadd/fsub already have a high likelihood of causing some large changes. Adding fneg is worth it though as that should be a simple operation.

This might lead to a fair number of changes. From looking at the main regression I found for example, it was a case of identical code being produced by the loop vectorizer, but scalar and vector costs being closer lead it to have a higher minimum trip count for entering the vector body (from D109368). The real problem there is aliasing causing LD4 loads to not be recognized though, so it's tough to see how this change is really to blame. There are a number of improvements too, to make up for that regression.

@fhahn do you have any thoughts on the change from your side, or any benchmark results that could be helpful?

In D145578#4201717, @fhahn wrote:

Just a heads up with are seeing a 10% regression caused by this change in a very SLP sensitive workload (the original source for the slp-fma-loss.ll tests). I still have to double check where the slowdown is coming from exactly.

Thanks for your help and reviews! I will commit this tomorrow if there will be no further comments.

Added context, fixed the test cases.

I have now benchmarked RAJAPerf too, which is a loop based and FP heavy benchmark. Results are neutral, mostly no changes.
So overall I am only seeing the uplift of this, with no regression.

I am wondering if you're trying to do too much in this patch. But I only had a quick look at this, and need to look again.
But my first request is going to be about the cost/bonus calculation. Since this is crucial for this transformation, it would be useful to document and comment the idea/algorithm at some place, with all the formulas and definitions (costs, bonus, latency, code size etc). I think this would greatly improve readability of these changes and the code in general.

High level question first, it looks like that geomean compile-times improve, and that is very surprising.
My guess is that one of the functional changes in findSpecialization is a good improvement on itself. Or maybe it is the caching of the CodeMetrics? Anyway, that's my question, just curious if you know what it is?

Looks like a good fix to me.

Thanks, I have restored that piece of logic and added the code-size check to it (and added a code size check to the test).
That means that we now get add an additional cost for TCK_RecipThroughput, as well as for TCK_Latency and TCK_SizeAndLatency.

I like it when I can delete things and achieve the same, so I have just done that. This was my understanding of your comments. Thanks for the suggestion and for looking into this.

Option --filter-short now accepts an optional arguments, and it defaults to 1.0s.
Some special care had to be taken if this optional argument is omitted, it then needs to recognise that the FILE arguments is not the optional argument to --filter-short, as also explained in the comments.

Thanks for the comments, I also like this idea!
I will prepare a new revision to support this.

(accidentally pressed enter)

In D145067#4162019, @dmgreen wrote:

Additional tests sounds OK. We don't always match splats to a single cost like we should.

Thanks, also for that case which I will add, that's interesting indeed.

Ok, cheers, and this looks very reasonable to me.

Looks reasonable to me.

The idea makes sense I think, but just to put things into context, do you already have a case or patch where we can see the benefit of this?

Thanks, and I will apply those changes before committing.

@dmgreen wrote in the other patch:

Thanks for looking into this.
I will let @RKSimon comment on the usage of the TTI hook.

I agree these tests are missing, so this is a good addition.

In D142359#4131731, @dmgreen wrote:

I was expecting that to be similar to the other fmul+fadd vs fma issue we have seen elsewhere, but I'm not sure it is. Does it reduce the value to a single element again?

As for a few examples, this case is a little worse. I'm not sure if it is using bad costs, but the slp vectorization seems to reach through a phi and the result when put through llc is mostly just more instructions: https://godbolt.org/z/z6hnEcPG1.
Another case which is perhaps simpler is this "distance" one from cmsisdsp: https://godbolt.org/z/1xz7GP3fM. It looks like something might be being scalarized, but again I've not looked into the details.

There are some nice improvements too, if we can get the regressions hopefully fixed.

Very nice.

In D142359#4083490, @dmgreen wrote:

The internal embedded benchmarks I tried had some pretty wild swings in both direction. I think it is worth working towards this, if we can try and minimize the regressions in the process. Running more benchmarks from SPEC and perhaps the llvm-test-suite would be good (maybe try and see what is going on in salsa for example? We might be getting the costs of scalar rotates/funnel shifts incorrect?)

There might be quite a few other cases. I can try and provide some examples if I can extract them.

Yeah, makes sense, cheers. LGTM too

The patch looks good to me, but I was just wondering if another approach would be to just match the sub(x, add(m1, m2)). pattern as mls, or is this easier/better?

We handle this by not making the choice during ISEL but instead selecting a pseudo instruction that gets expanded after register allocation when we have access to more information.

There is no precedent for:

• matching opcodes like that, the latest revision adds the splat_vector opcode check,
• checking the uses of sdnodes like that.

I experimented with replacing AArch64mul_p_firstOpndWithSingleUse -> AArch64mul_p_oneuse :

And in the meantime, can you upload a new revision with full context please, and rebase it on top of your change that precommit tests? Then we can see and discuss the changes this is making, and check if we see any improvements.

And for completeness, summarising previous comments, these are my other 2 concerns.

In D142656#4096082, @david-arm wrote:

Hi @SjoerdMeijer, in @sushgokh's defence there is precedent for some of the changes in this patch - by changing from SVE_4_Op_Pat to SVE_4_Mad_Op_Pat we are able to set the AddedComplexity field to the pattern, which is not dissimilar to SVE_3_Op_Pat_SelZero or SVE_3_Op_Pat_Shift_Imm_SelZero, i.e.

let AddedComplexity = 1 in {
class SVE_3_Op_Pat_SelZero<ValueType vtd, SDPatternOperator op, ValueType vt1,
                   ValueType vt2, ValueType vt3, Instruction inst>
: Pat<(vtd (vtd (op vt1:$Op1, (vselect vt1:$Op1, vt2:$Op2, (SVEDup0)), vt3:$Op3))),
      (inst $Op1, $Op2, $Op3)>;

class SVE_3_Op_Pat_Shift_Imm_SelZero<ValueType vtd, SDPatternOperator op,
                                     ValueType vt1, ValueType vt2,
                                     Operand vt3, Instruction inst>
: Pat<(vtd (op vt1:$Op1, (vselect vt1:$Op1, vt2:$Op2, (SVEDup0)), (i32 (vt3:$Op3)))),
      (inst $Op1, $Op2, vt3:$Op3)>;
}

What I don't fully understand is why the complexity has to be so high, since it suggests there are multiple competing patterns and it might be useful to understand what they are. I admit that AArch64mul_p_firstOpndWithSingleUse looks a bit unusual and I'm not sure that we should be checking for explicit opcodes such as TokenFactor, etc.

In D142998#4095916, @sushgokh wrote:

I tried seperating (1,2) and (3,4) as you say above. However, mla->mad is side effect of implementing (1) and this makes seperating (1) and (4) a difficult thing unless we introduce some hacks to do so

I think we need at least 3 patches:

and one more request, which I forgot to add earlier:

Ok, I misunderstood a few things, but see comment inlined.

First, I think the description needs some clarifications.
As I understand it, the problem description is that we would like to match/rewrite this pattern:

I will look at this soon.