Since you discovered this problem with non-default pass pipeline, then I do not need to see your test. With the default llvm passes, it was easy for me to find tests with load where the patch can be beneficial because of limitation in alias analysis preventing some optimizations and this patch can clean up things. But, I was not able back then to find meaningful test with non-load where this patch can help. It is great that you have a test (with your non-default passes) where this patch can be useful. It seems that both of us tested this code very well on ARM. Also, theoretically removing an extra instruction should be beneficial for other architectures too.

The patch looks good to me. It is a generalization of the original patch. The only thing missing here is to describe the testing results or post them here (even though on paper, getting rid of a truncate instruction should be beneficial).

What if the code is written with intrinsic but using mul, reduce (say similar to last test in this patch), then this patch will optimize that into dot product instruction. So, for legacy code that was written with old intrinsic, then this patch will remain useful even after dot product is implemented in the vectorizer.

There are few things missing in current work such as indexed dot product or what they call s/udot (vector, by element) in the ARM document (no need to do it now but a comment about that would help). There is also sve dot product. We need to port this code to SVE.

Some cleaning and update to comments.

In D49151#1174443, @efriedma wrote:

Your example doesn't really help make the case for this patch. The load in that test is actually loop-invariant; we just don't figure that out until after indvars transforms the induction variable. Probably LICM could be fixed to handle this case earlier. Then ultimately, the multiply goes away; the extra operation you're trying to get rid of is actually the sign-extension of a PHI node created by LSR. LSR and/or SCEV could probably be fixed so this produces an i64 PHI instead. Either of those fixes would be more straightforward and more obviously profitable.

Ping

In D49151#1158061, @efriedma wrote:

How does the code generation actually change on aarch64? As far as I can tell, you're basically just changing an "sxtw #2" to an "lsl #2"; does that save a uop on A72?

add LLVM codegen tests as suggested in the reviews.

Was not able to update this particular review with the new code, So I created a new one in https://reviews.llvm.org/D44591

Committed as r324940 and r324912

Question about the failures: I am now wondering if this means we were and still are missing tests?

In D41360#961869, @SjoerdMeijer wrote:

Looks reasonable to me.

Just checking, do we need those changes in CodeGen/arm_neon_intrinsics.c? I mean, trunk is already using i16 in these tests.

Code has been up-streamed in https://reviews.llvm.org/rL320123 and https://reviews.llvm.org/rL320204.

Thank you for providing several very useful feedback to improve this patch. In the future (not very soon), I am planning to make this pass more complete by 1) implementing the existing vector element sub-pass in the same way as optimize interleaved store instructions sub-pass for consistency reason (put all rewrite rules and not just one, caching, instruction replacement table, etc.) 2) Addressing the ST3 instruction inefficiency assuming I can find an efficient combination of instructions to replace ST3. I hope I can put you as the reviewer.

Added caching mechanism to save decisions per target of whether to go into the interleaved store replacement pass or not.

i

Code reuse and simplification due to the use of a table that has instruction replacement info.

Adding subtarget as part of key and simplifying some code.

Made caching mechanism for instruction replacement decisions to work across functions (instead of within function only) by declaring SIMDInstrTable as a global variable.

Hmmmm.... this seems to be a tough one to find the right tradeoffs....
My personal opinions/observations:

1. Making ShouldExitEarly stop the whole pass even if some of the optimizations would trigger is a hack that makes this pass operate in a non-obvious way and that will bite us in the future.
2. I guess there's a chance even more peephole-y patterns will be added to this pass in the future, which would make ShouldExitEarly even more expensive to check for all rewriting rules implemented. But also, if that function would take short cuts and heuristics, the pass would operate in more surprising and non-obvious ways.

The above made me wonder if to keep compile-time under control, it wouldn't be better to integrate this in one of the existing frameworks for peepholes rather than in a pass of its own. And then I looked at D21571 where this pass got introduced and saw such approaches were probably tried and rejected in early version of that review.
Right now, my best guess of the easiest way forward is to make ShouldExitEarly compute the profitability of all rewrite rules implemented, and let it cache the results of that analysis for each subtarget it encounters. That way, the cost of this computation should be amortized out over all functions the pass will handle, resulting in not having too much impact on compile time?
Having said that, I don't know of another pass that has a caching mechanism like this. But I also haven't tried looking for such an example. I'm not sure if there's going to be a gotcha in trying to cache those results somehow.
What do you think?

Thanks,

Kristof

A version that addresses the latest comments and in particular a more complete version of shouldExitEarly().

I thought the "shouldExitEarly" method aims to check if any of the transformations the pass can do can be profitable for the target at all.
My understanding is that this can be checked by just examining the instruction latencies, not having to look at the actual code being compiled at all. I assume that the saving of compile time comes from the fact that "shouldExitEarly" needs to do an amount of work proportional to the number of rewriting rules implemented, not proportional to the amount of code being compiled.
So, for the pass to work as expected, i.e. always run when for the target at least one of the transformations is profitable, I think shouldExitEarly should check all rewriting rules implemented.

Does that make sense, or am I misunderstanding something here?

Thanks. I assume you confirmed there are no performance regressions at all in the test-suite?

But my main question at this point is: did you benchmark this? What are the results of that? On which cores/benchmarks?

Kristof, thanks again for the new feedback. Uploaded a new version.

Kristof, Thank you for the feedback. Very useful comments to make the patch more readable and more maintainable.
I addressed your comments with the new version.

ping

https://reviews.llvm.org/rL305820

I am resubmitting the combination of D32511 and D34161 in here to avoid any confusion.

Ok, I will upload the combination of the two patches here.

In D34161#781106, @SjoerdMeijer wrote:

Not sure if I am missing something, but the new regression tests arm-v8.2a-neon-intrinsics.c and aarch64-neon-intrinsics.c are failing for me (after first applying patch D32511 and then this one here in D34161). For example, the very first test in arm-v8.2a-neon-intrinsics.c checks that the function argument is <4 x half> %a, but <4 x half> %1 is generated. With anything other than -O0 you will get the %a (and then it becomes a tailcall).

Removed unnecessary f32 intrinsic tests; removed tabs