Yay, one more postinc, LGTM.

LGTM

I also think this must be a binary op.

Thanks for fixing, LGTM.

LGTM

LGTM

LGTM

Ok, cheers, LGTM

Not that easy to spot that this is an improvement from these test changes, but I think I see a few, and the idea makes sense, so LGTM.

Looks like a good fix. One quick question: I see that some CPUs have FeatureAlternateSExtLoadCVTF32Pattern set. Is that something we want too?

I haven't addressed feedback yet, but have just added a few things to help with the discussion where this belongs.
This now also prints the number of bytes per instruction, so for thumb it now distinguishes between the narrow/wide encodings, and I have also added printing a histogram of immediates. This will look now look like this:

https://discourse.llvm.org/t/llvm-objdump-print-instruction-histogram/62333

Thanks all for looking at this!

Thanks for explaining! Makes sense, LGTM.

Looks reasonable.

Thanks for the introduction to perfect shuffles! :-)

That's quite a lot change in the test cases. It's easy to see that the smaller ones are improvements. For the bigger changes that isn't that obvious. But I trust you have run numbers and this is overall better. So LGTM, let's give this a try.

Sounds reasonable.

Looks like a good one to me.

Thanks for fixing. LGTM too.

It's looking good to me too, just a last round of nits (inlined) and one question just to double check: is the SPEC score the same? I.e., do we still specialise what we want to specialise?

In D119880#3380014, @labrinea wrote:

That's true; the new formula is not in the current revision. The idea is to keep a sublinear number of specializations when the number of candidates grows enormously (not expected to happen in real life code). So imagine we had 1000 candidates n/2 would be 500 whereas log10(n^4)+1 will be 13. I measured the instruction count of clang when compiling the llvm test suite with log10(n^5)+1 ( this function has a steeper curve - see https://www.google.com/search?q=plot+log10(x%5E5)%2B1 ) and it had a significant impact on ClamAV (1% more instructions over baseline compared to 0,57% increase with log10(x^4)+1).

In D119880#3375169, @labrinea wrote:

I've tried another formula to determine the amount of speciazations we are keeping (instead of Sorted.size()/2). It is defined as auto NumSpecKept = (size_t)std::log10(std::pow(Sorted.size(), 4))+1;.

LGTM

Sorry for the delay. I need to look a bit more at this, but I added some first thoughts inline.

Looks like a nice change to me.

Thanks, looks like a good change to me.

Looks like a good refactoring, but needs a clang-format, and have added one question inline.

Sorry for the delay. First, about a NFC and some refactoring, can the reshuffle of ArgInfo and SpecialisationInfo and the changes in the Solver functions be an NFC change perhaps?

Thanks, looks good!

Ah, sorry, forgot about this. Can you upload the patch with some more context please so I can have a quick look again?

Ok, thanks, looks like a nice patch to me.

Clarifying my previous comment a bit more:

There is quite a lot to unpack here:

Yeah, nice patch, thanks.

Gentle ping. Any further thoughts on this?

In D118591#3290167, @fhahn wrote:

In D118591#3290145, @labrinea wrote:

I have heavily modified the original reproducer, but the problem is that we don't have buildbots running the llvm regression tests with asan. I can only see two builders for libc with asan here https://lab.llvm.org/buildbot/#/builders. Do you think there's still value in adding this testcase?

There should be multiple bots that run with various sanitizers, e.g. https://lab.llvm.org/buildbot/#/builders/168

In D118591#3286952, @labrinea wrote:

I was wrong when I said we are not testing the removal of dead instructions. I was looking at the tests under the original commit D93838, but we have actually added more tests since. For example function-specialization-constant-expression.ll is exercising this code path.

Agreed, lazily removing the instructions in this way is what I was expecting.

Okay, thanks, that sounds like a good plan!

Fresh numbers: https://llvm-compile-time-tracker.com/compare.php?from=0776f6e04d8c3823c31b8fa6fa7d1376a7009af1&to=9522ad62a3f461bc989d2cca3c2bf09b6dab4632&stat=instructions

LGTM

Hi @asbirlea, thanks for checking, much appreciated! About:

Looks like a good change to me.

I did leave the legacy behavior as an option since a) loop-flatten stops triggering if I extend due to weirdly specific pattern matching I didn't understand and b) we could reasonably use the mode if we'd externally established a lock of overflow.

Hey, it's me again. :)
Any plans to pick this up, shall we get this in?

Thank you for resolving all the issues raised! Could you rebase this patch on ToT? (to include the MSSA update and the move to LPM1?)

With your help, we have fixed quite a few things, like updating the MemorySSA state and moving it a LoopManager (and fixing a performance regression related to that).

In D116612#3254116, @dmgreen wrote:

I'm happy so long as the performance remains OK! Does this need to be rebased now?

@dmgreen : You reverted "rG86825fc2fb36: [LoopFlatten] Move it to a LoopPassManager" because of a downstream regression. With D116660 now accepted/committed, that unblocks this change. This should address this regression, so are you happy with this?

In D116660#3252608, @asbirlea wrote:

Changes LGTM.
Nit: clang-format.
A simpler way to check all verification is now to pass VerificationLevel::Full to verifyMemorySSA(); I'm mentioning this for your local testing, in case your build doesn't have EXPENSIVE_CHECKS on already, not to include in this patch.

Sorry, my bad, should be fixed now.

Thanks for the review, sorry for the delay.
Addressed comments and have added that to the LPM.

Ping

Ping

Ping.

I somehow missed the failing regression test.
Turns out I do need to check AR.MSSA for passing on the MSSA object (or a nullptr).

slightly less & more straight-forward code?

I decided to do the updating of MSSA separately in D116660 (and will remove it from here).

Ah, forgot to mention tests. Without the update MSSAU->removeEdge(), I was expecting AR.MSSA->verifyMemorySSA() to run in an assert (with the expensive checks enabled in my build), but somewhat surprisingly to me it didn't.
So not sure yet if we need tests for this, but am looking into this.

Thanks for the pointers! I have added a PhaseOrdering test, while I now look into updating MSSA.

In D116612#3220448, @lebedev.ri wrote:

Does LoopFlatten not make use of SCEV?

I think all points have been addressed, so am looking for an LGTM .... anyone interested? :-)

In D109958#3214802, @nikic wrote:

New compile-time: https://llvm-compile-time-tracker.com/compare.php?from=6f45fe9851c673883b3a258351ee4997aa2c028c&to=439d1c9613828db24ad03eb415baad2e76b8913c&stat=instructions Looks ok to me.

Update of the tests after D110057.

With D110057 committed, is this now good to go too?