Restructure checks as suggested and moved the assertion to lambda, thanks!

We also had user reports hitting the issue fixed by the change in the default. I added a relatively small test case that shows the binary differences caused by a single .loc: 0bd5bbb31e03 Perhaps this is helpful for anyone wanting to take a closer look at the underlying issue.

In D104188#2823036, @Ayal wrote:

Good catch! Please remember to add a testcase along with D100260.

LGTM thanks for the fix Steven!

In D102478#2817768, @erichkeane wrote:

Just a couple of nits here, basically see how much we can put in the 'cheap to check' branch.

ping :)

Thanks for the update! Could you update the description/title, as they seem a bit out-of-sync after the recent changes.

Thanks for the patch! some comments inline.

Thanks for the patch. Some comments inline

After this change, building the LLVM test-suite for X86_64h seems to fail http://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-x86_64h-O3/9586/ :

In D104126#2813621, @Meinersbur wrote:

Does this expect on loop unrolling to happened? Did you consider adding #pragma clang loop unroll(full) to make sure of it?

In D103816#2816343, @jaykang10 wrote:

@fhahn @jonpa I am sorry for ping.

Address remaining comments, added additional tests, clarify comment on why we need to skip recipes used by first-order recurrences.

Thanks for the initial comments! Sorry for the delay with following up.

Following patches will utilize the LoopNest structure for more efficient optimization.

In D104179#2815159, @eopXD wrote:

In D104179#2815155, @fhahn wrote:

Following patches will utilize the LoopNest structure for more efficient optimization.

Would it be possible to provide some data on the efficiency improvements?

Hi @fhahn ,
For example, given a perfectly nested loop-pair and we have matched a store operation that we can hoist to the inner-loop's header as strided store operations,
we can further check on the outer-loop, see if the outer-loop SCEV maintains the continuous memory access and hoist the operation one more loop outwards.

Thanks for all the comments Ayal! I updated the code and removed some redundancies as suggested.

Following patches will utilize the LoopNest structure for more efficient optimization.

It looks like this is causing build failures on certain macOS / SDK combinations, e.g. http://green.lab.llvm.org/green/job/lldb-cmake-standalone/3288/consoleFull#-195476041949ba4694-19c4-4d7e-bec5-911270d8a58c

In D101970#2813643, @Carrot wrote:

The fix has been committed as f35bcea1d4748889b8240defdf00cb7a71cbe070.

Finished off the main functionality, update includes

• computing & comparing the cost of the versioned block to the original block,
• more tests,
• preserving LI & DT if changes to the CFG are made.

Without the fix the testcase crashes.

I also suspect this patch is causing the following failure (with -mllvm -verify-machineinstrs ) on GreenDragon (http://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-x86_64-O3/9579/)

I'm not quite sure why the implementation is in llvm/lib/Transforms/Scalar/FunctionSpecialization.cpp, rather than llvm/lib/Transforms/IPO/FunctionSpecialization.cpp? It's interprocedural only, right? The implementation in SCCP.cpp works on both a single function and a module, that's probably with its in the Scalar sub-directory. It's also a separate pass from IPSCCP, so I am not sure if the pass definition should be in llvm/lib/Transforms/IPO/SCCP.cpp.

In D99424#2709964, @nikic wrote:

Okay, I've finally gotten around to looking into this in more detail. Because thinking about this really fries my brain, I ended up modelling this in alive2 (hopefully correctly...)

https://alive2.llvm.org/ce/z/HYBxGs This is the general case and shows that we only need nsw flags on the arithmetic, rather than both nuw and nsw. This makes sense, as all the arithmetic involved is signed.

https://alive2.llvm.org/ce/z/qXicaB This shows that it is indeed safe to always take the power-of-2 portion of the scale, even if the arithmetic is not nsw.

In D103597#2808045, @MeeraN wrote:

@fhahn in response to your inline comment about performing the renaming registers check twice, if we don't have it we won't try to rename the registers at all, since the check on line 1764 is not reached in our case as we never enter the if statement that contains it. So we brought it earlier to make sure that it tries to rename the registers, if it can. I'll follow-up the rest of the comments in a new diff.

In D93838#2807597, @SjoerdMeijer wrote:

In D93838#2807515, @fhahn wrote:

In D93838#2793238, @SjoerdMeijer wrote:

Addressed @fhahn 's comments: don't run the solver for specialised functions removed the recursive specialization test for now.

I'm not sure if removing the recursive specialization test is the best thing to do, without known what it is supposed to test? If it is a legitimate test, I thin it would be good to keep it.

Thanks for the comments. Since they are minor, I will fix it before committing. I.e., will remove that update to solver and add the test back.

Rebase & ping :)

In D93838#2793238, @SjoerdMeijer wrote:

Addressed @fhahn 's comments: don't run the solver for specialised functions removed the recursive specialization test for now.

Please update the description of the patch with a motivation for the fix and why it is the right thing to do. This will make the change easier to review.

Update to remove deleted instructions early, so we do not pick up instructions to be deleted during SCEV expansion, flip branch targets.

In D103877#2805474, @mkazantsev wrote:

As far as I understand what is a nsw for multi-argument add, it means that there should be no wrap in between if we add the operands one by one in any order. Imagine the case when a = c = SINT_MIN and b = d = SINT_MAX. We can say that a+b has <nsw>, c+d has <nsw>, and (a+b) + (c+d) also has nsw. But if we say that getAddExpr(getAddExpr(a, b, nsw), getAddExpr(c, d, nsw), nsw) = getAddExpr(a, b, c, d, nsw), it's not true if we compute the latter as a + c + b + d. It will overflow on a + c already. Is my understanding wrong here?

Updated to also add the -debug-pass-manager option, so it is easier to check that -use-new-pm works as expected. I also pinned the remarks tests to use the legacy PM and updated llvm/test/ThinLTO/X86/newpm-basic.ll to check that the new PM is run via the debug output.

In D103255#2796605, @reames wrote:

LGTM as well. I'm not super familiar with this code, but I skimmed the callers and the necessary conditions seemed to be locally implied.

As an aside, do you have a test case where SCEV can't figure this out? I've got D103118 out for review now, and am interested in other such cases.

This does not seem memcpy specific. If that's the case, could you please update the title/description of the patch? Also the test cases in llvm/test/CodeGen/AArch64/memcpy.mir seem to specifically test additional renaming cases, so it might be good to put them into the existing llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir or into a file named along those lines. it would also be good to include a brief comment with the tests what is different to the existing re-naming tests.

Rebase on top of main, so it can be landed without requiring the parent patch

Fix crash caused by not updating the DT's DFSNumbering and case where we checking for aliasing between pointers of the same underlying object.

In D102834#2802512, @SjoerdMeijer wrote:

In D102834#2802402, @fhahn wrote:

In D102834#2793483, @SjoerdMeijer wrote:

Hi Florian, thanks for putting this up for review and starting the discussion. If you don't mind me asking, how high is this on your todo-list? The reason I am asking is that this well help x264 where we are behind a lot (and it more in general it solves this long outstanding issue that we have). Fixing x264 is high on our list, which is why I put up D102748 to start this discussion. If, for example, you don't see time to work on this, we could consider looking at it.

It's fairly high on my todo list, as we have quite a number of of such cases reported by our internal users. But I'm not planning to rush and I think we should also start collecting micro-benchmarks for the missed opportunities, so we can thoroughly evaluate the impact and do not introduce regressions in the future.

Nice one, excellent. What are your ideas on the micro-benchmarks? Is that something you'll be using internally, or were you e.g. thinking about adding things to the test suite. Just asking because I would be happy to contribute to that.

In D99354#2763024, @jaykang10 wrote:

The below bugs are fixed.
https://bugs.llvm.org/show_bug.cgi?id=50279
https://bugs.llvm.org/show_bug.cgi?id=50302

In D102834#2771053, @ABataev wrote:

You can easily get the first instruction - VectorizableTree.front(). Stores can be only in the front of the vectorizable tree. As to the last instruction, I think we need to scan all tree entries and find all gather nodes with memaccess which may alias with stores. The main problem that the versioning better to perform before we try to build the tree, otherwise we may decide to gather some instructions instead of trying vectorizing them because of possible aliasing. And it may not be profitable.

I'm seeing crashes when trying to build 471.omnetpp with -O3 -flto on X86 when running the pass just before the vectorizer (as below). Please take a look.

LGTM, thanks!

Makes sense to me! Do we need to update LangRef?