Thanks, LGTM!

Sorry for inconvenient, let me add a comment in commit message.

I think I managed to figure out what's going on here in the end, but it would help to explain what the bug is in the commit message, something like:

@nikic this fixed the regression, thank you!

Hi, we've got a ~6% regression in SPEC INT 2006 462.libquantum on AArch64 (both -flto and -Ofast) that comes back to this change. See here for a reproducer https://godbolt.org/z/dq98Gqqxn (-fno-vectorize is not strictly necessary, but it does make the difference easier to spot). @dmgreen mentioned to me that we could probably fix this up in the AArch64 backend, but a fix in the mid-end might be more generally useful too.

Thanks for the comments. I'm happy to leave LoopAccessAnalysis alone for now and focus on Loop Distribute's cost model. I was hoping that a cost model tweak or two might enable some more loop distribution in TSVC, but it looks like that isn't the case.

Looking at TSVC a bit, @xbolva00 :

• s221 won't distribute because the second read of a[i] is removed by EarlyCSE, so there is no unique load instruction for a second loop. For this loop I'm not convinced that distribution is likely to help performance; it's a trade-off between (some) vectorization and re-loading both a[i] and d[i].
• s222 also gets mangled by EarlyCSE, but the result would still be distributable if it weren't for the order of the stores to e[i] and a[i]. This runs into a limitation of LoopAccessAnalysis, which can't reorder instructions. Perhaps it could help to do a bit of scheduling on IR?
• s2275 as mentioned above, this runs into another LoopAccessAnalysis limitation: it only handles innermost loops. I'm not sure how easy (if at all possible) it would be to lift that restriction.

Now, there are no differences in distributed loops in the test suite and SPEC, before and after the patch, as intended.

Rebased, and addressed discrepancy in the loop distributed. The difference hinges on loops that contain backward dependences which the loop vectorizer can handle, but which would frustrate loop distribution. In this case, we don't distributing the loop and leave it to the loop vectorizer.

In D100381#2685736, @sanwou01 wrote:

IIRC SPEC was neutral except for the expected gain on hmmer, but I'll re-run them with the current patch.

In D100381#2688791, @fhahn wrote:

We also have some target-specific heuristics for loop-distribute, which focus on the number of memory streams a CPU can handle IIRC. I never got around posting them upstream so far. Let me go back and look at those heuristics.

In D99596#2663702, @SjoerdMeijer wrote:

By just looking at this patch I find it a bit difficult to get an overview of all moving parts involved. I.e., this makes probably sense:

Loop distribute bails out early if a loop is already vectorizable.

but by not doing this, do we remove opportunities for the vectoriser? So, perhaps the easiest is to get some perf numbers on the table?

Then, we can think about the cost-model too, and see if we can create some ideas about that. This pass is not enabled by default, so if perf numbers are okay and we don't make (downstream) users of this pass unhappy, it looks like a good step forward to me, but some ideas about steps after that would be good.

In D100381#2685619, @SjoerdMeijer wrote:

That certainly looks encouraging. Just some remarks about the perf numbers first. I found that the llvm test suite can be quite noisy and you certainly need to restrict it to the subset of CTMark for some more meaningful numbers. Just checking, did you do this? Because looking at all tests, I think I see more tests than I would expect with CTMark, but I could be wrong. How about a SPEC run for something that runs a bit longer?

Since this is mostly just moving pre-existing code, I think it's fine to address the style issues in a separate NFC commit. I believe all comments have been addressed, but please wait a day or two before committing in case there is anything else. LGTM.

+1 on eagerly awaiting a fix. a 3% regression on astar (AArch64, LTO) bisects to @lebedev.ri 's revert: https://reviews.llvm.org/rG6270b3a1eafaba4279e021418c5a2c5a35abc002 .

Thanks! This seems to be moving in the right direction. Now that SCCPInstVisitor is separate, its declaration can move into SCCPSolver.cpp entirely.

Good spot! Thanks!

I think the issue here is that quite a number of implementation details are leaking into the header file where they don't belong because they are not part of the public interface. You're right that code using the header file doesn't have access to these by virtue of being private, but this does not make for a very readable header file, IMO.

@spatel @lebedev.ri thanks for the comments so far. Any other comments, or is this okay as is?

Add a few more tests and some further test reduction.

In D97397#2594000, @lebedev.ri wrote:

I'd like to see some test improvements:

1. There are no tests with extra uses on bitcasts
2. There are no tests with something like

%xb = bitcast <2 x half> %x to <2 x i16>
%yb = bitcast <2 x bfloat> %y to <2 x i16>
%r = shufflevector <2 x i16> %xb, <2 x i16> %yb, <4 x i16> <i16 3, i16 2, i16 1, i16 0>

I suspect this will miscompile?

3. Some tests have unneeded stuff. They should only contain 2 bitcasts and a shufflevector (and a ret), nothing more.

Address comments.

In D97397#2585516, @spatel wrote:

We intentionally do not create new shuffle masks in instcombine because we can't guarantee that codegen can lower arbitrary masks efficiently, but this patch seems fine since it just re-uses the existing mask.
If there is motivation to handle casts of different-sized elements (and therefore requires a new mask), you might look at building on VectorCombine::foldBitcastShuf(). We use the cost model there to avoid creating unsupported shuffles.

Committed as https://reviews.llvm.org/rG93d9a4c95aff . Looks like I forgot to add the "Differential Revision:" line :(

I've adopted your suggestions, thanks!

This looks good to me. @fhahn are you happy with the suggested approach to tighten up the SCCPSolver class in subsequent patches, when function inlining starts using it?

Maybe I'm missing something, but this change doesn't seem to be effective anymore after the new pass manager switcheroo. Did this pass not get ported, in favour of SimpleLoopUnswitch? This now shows up as a regression relative to (what will be) LLVM 12.

This looks like a fine first step to separate SCCPSolver from SCCP. I agree with @fhahn that the SCCPSolver class should be improved by pulling out implementation details into (separate, hidden) classes, but this is probably best reviewed in separate patches. At least it looks like the public interface of SCCPSolver is already restricted to what is needed by SCCP.

Hey, we've run into a case where this patch causes a dead loop to appear which doesn't subsequently get removed. It's not a huge deal (we're seeing some great speed-ups from this patch too!), but a niggle that might be nice to address. Reproducer:

As the approach here is pretty much identical to D36432, do you have any plans on the cost model for this pass? The earlier proposal seems to have been abandoned due to a lack of good heuristics to trade off between the (potentially huge) increase in code size (as well as compile time) and performance improvements (which may or may not arise due to later optimizations). Perhaps it would help to look at what trade-offs GCC makes? Specifically, it would be good to have an idea of how you would implement getSpecializationCost and getSpecializationBonus.

Looks good to me!

In D94232#2503921, @fhahn wrote:

In D94232#2500595, @sanwou01 wrote:

[...]

Yes, I think the only thing outstanding with respect to testing is the omnetpp_r failure. Can you confirm if that is caused by the patch or not?

And any review of the patch would be appreciated of course!

In D94232#2498855, @fhahn wrote:

In D94232#2498633, @sanwou01 wrote:

So on SPEC 2006 this fixes astar (+9.6%) as well as shakes things up enough to "fix" h264ref (+9.0%, see D93946). Other notable changes are libquantum (+3.2%) and omnetpp (-2.8%). Geomean is +1.5%.

Thank you very much for running the numbers!

Just to double check, positive here means good (I assume increase in score)?

So on SPEC 2006 this fixes astar (+9.6%) as well as shakes things up enough to "fix" h264ref (+9.0%, see D93946). Other notable changes are libquantum (+3.2%) and omnetpp (-2.8%). Geomean is +1.5%.

Turns out the failing benchmarks are due to a miscompilation with -g3 (which we add to profiled runs). The patch does seem to make that miscompilation more likely. I'll try to reduce that separately, but at least I'll have some performance numbers shortly.

In D93946#2493599, @sanwou01 wrote:

In D93946#2493440, @aeubanks wrote:

Could you do some initial investigation or come up with a repro? Maybe an optimization remark that doesn't fire after this change?

Yes, I will do an initial investigation, hopefully resulting in a reproducer. It is always a bit of a challenge with LTO benchmarks, but the regression is large enough that it should be easy to spot.

In D93946#2493440, @aeubanks wrote:

Could you do some initial investigation or come up with a repro? Maybe an optimization remark that doesn't fire after this change?

Hi folks, I'm afraid an 8% regression in h264ref (SPEC INT 2006) bisects to this patch. This in on AArch64 with -flto on a Neoverse-N1 based system, but I wouldn't be surprised if this showed up elsewhere too.

Yep, definitely a problem on our end; I've got similar symptoms in other runs. Sorry about the noise, and I will try to get some perf data once we sort this out.

Our flags are -mcpu=native -O3 -fomit-frame-pointer -flto on a Neoverse-N1 based system, so -mcpu=neoverse-n1 should do the same thing.

@fhahn I'm afraid I'm seeing runtime and verification errors on perlbench, gcc, gobmk, and astar in SPEC INT 2006. I'm guessing something later in the pipe really doesn't like it when certain loops aren't rotated?

Thanks, Florian. I'll give the patch a run through our benchmarking.

@pzheng I don't have perf data for Cortex-A57, but I would certainly expect to see an uplift there too.

In D84108#2422611, @fhahn wrote:

I had a first look and it seems like there's some bad interactions with the LTO pipeline. The problem in this case is the following: we now rotate a loop just before the vectorizer which requires duplicating a function call in the preheader when compiling the individual files. But this then stops inlining during LTO. I'll look into whether we should avoid rotating such loops in the 'prepare-for-lto' stage.

Hi, our flags are "-Ofast -flto -mcpu=native -fomit-frame-pointer", where "native" is a Neoverse-N1 system. Let me know if that helps.

Just wanted to add to @fhahn that we're seeing this too, as an even more notable 8% regression (same benchmark, different hardware). Florian's explanation matches what I'm seeing, but I hadn't had a chance to confirm what was happening.

Hi, this is regressing a few internal workloads (physics simulations, AArch64) by a few percent. Did you do any performance measurements for this change?

Reverted due to new test failing on a bunch of buildbots. I'll try again tomorrow, looks like the other pipeline tests manage to work around it.

@SjoerdMeijer yeaahhh these pipeline tests are a bit of a pain, but nothing some big-brain sed scripting can't solve.

Added LTO pipeline test

Thanks @tpopp, that'll unblock all of us.

In D88423#2298681, @efriedma wrote:

I'm not sure I understand the reason behind moving the EnumerateType call from incorporateFunction to the ValueEnumerator constructor. We don't walk the attributes before that, do we?

Committed as d5fd3d9b903e

SPEC 2017 on AArch64 is neutral on the geomean. The only slight worry is omnetpp with a 1% regression, but this is balanced by a .8% improvement on mcf. Other changes are in the noise.

I know this has already been reverted but just FYI that I've bisected a ~2% regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is due to the extra unrolling / cost modelling issue already mentioned?

Fix for when there is no fp16 faddp + testing

LGTM, thanks for fixing this! Could you wait a day or two before committing to allow others to comment?

Extend to f16, f32, f64 and i64

Rework to match faddp in AArch64 ISel lowering

Thanks for the feedback. I agree that ideally we'd be generating reduction intrinsics in IR and matching that in the backends. I don't think the pairwise add can be represented with the current intrinsics though: we'd need a <2 x float> variant, or a predicated version of the <4 x float> intrinsic to do this for strict FP math, I believe.

Thanks @spatel . You're right that we miss that pattern, but, so does x86 currently it seems (I don't read x86 very well so I might be wrong). Using your faddp example: