In D94232#2504325, @sanwou01 wrote:

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!

(I thought I posted this on Friday, but looks like I forgot). The omnetpp_r crash on SPEC 2017 has not been seen again in subsequent runs so this might have been a fluke. Performance on the new runs looks fine, no change on omnetpp_r.

In term of review, could you add a test case? A -prepare-for-lto flag for loop rotate for testing purposes seems reasonable to me.

This unfortunately breaks a bunch of existing projects. I'll abandon the change for now, as the gains are probably not worth the cost of breaking backwards compatibility.

This surfaced in more fuzzier invocations, I went ahead and submitted a fix that avoids adding empty/tombstone keys when adding new interleave group members: 83aa93e99542

This ensures that the Clang loop pragma vectorize_predicate([enable|disable]) is ignored
when vectorize_width(1) is also used, since that effectively disables vectorization.

IIUC this is similar to the handling of partial overwrites in DSE (https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp#L548)?

In D94232#2500595, @sanwou01 wrote:

In D94232#2498855, @fhahn wrote:

In D94232#2498633, @sanwou01 wrote:

On SPEC 2017 there is still a problem with omnetpp_r (runtime error), which I'll have a look at next, but is otherwise neutral on geomean with only minor wobbles (+1%, -0.5% at most) on individual benchmarks.

With this having a substantial impact on performance, I'm quite keen to help get this in soon, ideally before the LLVM 12 branch, so please let me know if there is anything else I can do to help.

Sounds good! I think the theory behind the change should mean it should be quite safe and the data so far seems encouraging; I can confirm similar improvements for astar on our tracking.

It would probably good to get this in well ahead of the 12.0 branch, so it has a chance to make it through various downstream perf-tracking systems.

That's great! Agreed that this could do with a little soak on main before 12 branches, which IIRC is scheduled for 26 January?

In D93764#2496964, @jonpa wrote:

If a dead path in a loop is unswitched into an empty loop, I suppose the idea is that LoopDeletion will later then delete it?

enable vectorization unconditionally, to be in line with the behavior in LTOBackend.cpp.

In D94783#2501324, @tejohnson wrote:

Agree, these should just be removed. but I also don't see any uses of disable-lto-vectorization in a test, can that and the corresponding DisableVectorization parameter be nuked as well?

In D94106#2480386, @nikic wrote:

In D94106#2480348, @fhahn wrote:

In D94106#2480229, @jdoerfert wrote:

In D94106#2480220, @nikic wrote:

Happy to see this issue finally fixed :)

I think the general approach here should be more along the lines of infering WillReturn inside FuncAttrs, and then just checking the WillReturn attribute here. Especially for languages which have no forward-progress guarantee it is important that WillReturn actually gets inferred somewhere, otherwise intrinsics will be the only thing that set the attribute.

Maybe if we turn on the attributor for a few selected attributes only?

I think it would be good to approach this incrementally. So start with gradually making the behavior 'more' correct than it is at the moment, while not regressing on performance. Treating functions without side effects without mustprogress as 'alive' seems like a relatively safe thing to do as a start.

We should also improve the inference, but to make progress it might be be desirable to not predicate one on the other to start with. I realize that having good-enough inference to start with is the more principled approach and I am not sure if the desire to make progress outweighs that in the case at hand.

I like incremental improvements, but I don't think this really qualifies. What you are doing here is not making the behavior more correct while not regressing performance -- you are making it more correct while not regressing performance for languages with a forward-progress guarantee. What's likely going to happen is that this lands because it works well enough for C++, and then the remaining and technically harder part gets forgotten...

Do not infer willreturn for __memcpy_chk.

In D90328#2499556, @fhahn wrote:

In D90328#2499373, @nikic wrote:

Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=a3904cc77f181cff7355357688edfc392a236f5d&to=b5ccbbb5b1d8fc76e2291a860e4cf0a4787b77d4&stat=instructions Seems mostly fine, only outlier is kimwitu++ in NewPM-O3 configuration (k.cc has 1.25% regression).

Something I don't understand about the general approach here is why we start at the Def store and walk all uses to find a Use store to eliminate. Wouldn't it be more efficient to start at the Use store, and check the defining access? With the current approach, I think we'll only find the cases where Def is the defining access of Use, as everything else would be an indirect use. This approach should also be much easier to make more powerful (by using getClobberingMemoryAccess()) without running into compile-time issues from visiting indirect uses.

I don't remember if there was any particular reason to choose this approach to start with. I can't think of anything right now. Let me see if there are any cases that we would miss if we flip things.

In D90328#2499373, @nikic wrote:

Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=a3904cc77f181cff7355357688edfc392a236f5d&to=b5ccbbb5b1d8fc76e2291a860e4cf0a4787b77d4&stat=instructions Seems mostly fine, only outlier is kimwitu++ in NewPM-O3 configuration (k.cc has 1.25% regression).

Something I don't understand about the general approach here is why we start at the Def store and walk all uses to find a Use store to eliminate. Wouldn't it be more efficient to start at the Use store, and check the defining access? With the current approach, I think we'll only find the cases where Def is the defining access of Use, as everything else would be an indirect use. This approach should also be much easier to make more powerful (by using getClobberingMemoryAccess()) without running into compile-time issues from visiting indirect uses.

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%.

I think this looks good now. I am planning to submit the patch tomorrow or so, unless there are further comments.

Updated to skip self-reads and simplified the code a bit.

In D90328#2489933, @dmccrevan wrote:

In D90328#2484275, @fhahn wrote:

reverse ping.

I think this is almost ready, it would be great to get the last remaining comments with respect to wording & tests wrapped up.

If you won't be able to follow up on this, it would be great if you could let us know, so potentially someone else could pick it up.

Sorry, still in a busy time.

I switched the name of the function to eliminateRedundantStoresOfExisitingValues. When I reverted my changes in some of the test files, one test now fails unexpectedly. The only reason those changed was because I ran that python script to update the tests.

For the additional tests, if someone else would like to pick that up, I am more than happy to go with that. I am not as well versed with my LLVM IR as many of you are :-).

I am not too familiar with how calls are handled in the X86 backend. I would really appreciate any pointers to cases that this approach might miss.

ping :)

ping

Rebased after f638c2eb4ee6d0a0bd0e80cd305ad93e382db8f5, which removed the anonymous namespace (it just included function definitions), thanks for the tip!

We have found that the calls to verifyFunction can take up to 66% of the time spent in llvm::LoopVectorizePass::processLoop. For some source files, verifyFunction took around 10% of the total compile time.

In D94230#2484413, @joechrisellis wrote:

In D94230#2484200, @fhahn wrote:

It seems there is already a pass to optimize SVE intrinsics. Is there a reason to add a another separate pass, rather than extending the existing one?

Hi @fhahn -- the reason I created a new pass here is because SVEIntrinsicOpts.cpp works on an instruction basis -- i.e. look at each SVE intrinsic call in isolation and see what can be done locally from there. On the other hand, the optimisation implemented by this pass instead requires you to look at the set of ptrue intrinsic calls in each basic block, which is not so easy to do within the existing structure of the sve-intrinsic-opts pass. I think it's probably cleaner to add a new pass to handle this, but I am open to suggestions. 😄

Thanks for taking a look!

Address comments, also add NumWillReturn stats counter and Function::willReturn()/Function::setWillReturn.

In D94487#2492933, @tejohnson wrote:

Thanks, I'll take a look hopefully today. As I just mentioned in the bug, ThinLTOCodeGenerator has the same issue - will you be migrating that as well?

The logic in general seems fine to me, but I'm not too familiar with the attributor internals, so it would be good for someone more familiar to take a look.

In D94494#2492801, @markus wrote:

bbi-51646.ll1 KBDownload

The issue was found with attached input bbi-51646.ll If necessary it could be added as a regression test but then the question is where (i.e. should it piggyback on some other file).

LGTM, thanks!

ping

Thanks! Addressed the nit and also added an option to limit the number of memory accesses to explore, to avoid excessive compile-times in degenerate cases

Rebase & ping :)

Thanks for putting up the patch!

Great, thanks for taking a look and the very helpful comments!

In D93662#2487498, @vsk wrote:

Looks good to me as-is. Thanks!

rebase after parent patch landed.

In D94175#2487083, @gilr wrote:

LGTM, with a nit.
(Seems reduction might be worth splitting widenPhiInstruction() and its own Recipe, but that's beyond the scope of this patch)
Thanks!

LGTM

In D86841#2484705, @atmnpatel wrote:

I'm happy to add a patch amending this, the reason it wasn't done that way was because at the time and even now, out of icc/clang/msvc/gcc, gcc seems to be the only one that happily removed such loops in C++ (https://godbolt.org/z/W9vj99), and I didn't have a particularly strong opinion on which way we should lean at the time.

FWIW I think it would be good to have a bit more details in the description for changes such as this, like a link to the public docs for the extension.

In D93734#2478272, @jonpa wrote:

I found out quickly that perhaps the hardest part of this was to update datastructures after changing the CFG in a loop... I have barely been able to build the benchmarks as it is, so I am in need of some good advice on how to update things after a loop change, for this patch to be usable. Currently I have changed things temporarily so that LI, DT, etc are recomputed after loop deletion (BTW, I found that only recomputing those analyses on master after LoopDeletion was not NFC, which surprised me... Is that a bug or expected with the aim to save compile time?)

Statistics on SPEC-17 on SystemZ:

Sent mail to llvm-dev (https://lists.llvm.org/pipermail/llvm-dev/2021-January/147678.html), to get thoughts on the 'speculatively create IR and throw away if not needed` pattern in general

rebase on top of the recent changes.