This should probably be split up into BasicAA (+unit test), MemoryDependenceAnalysis(?), and GVN patches.

Please precommit tests.
This is more like it.

InstCombine is a canonicalization pass, and by design it must not make use of TTI.
This should be fixed by a reverse transformation in backend.

Tests missing

Seems about reasonable to me.

Why do we even want to bend over backwards and try to handle broken code correctly,
at the cost of regressed performance for everything,
instead of just adding defensive asserts against such problems,
and letting/telling the user fix the code?

In D94576#2495791, @lebedev.ri wrote:

In D94576#2495787, @tislam wrote:

In D94576#2495786, @lebedev.ri wrote:

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

Hi @lebedev.ri, the first profile is with the vanilla llvm.

That is what the message said yes.

In D94576#2495787, @tislam wrote:

In D94576#2495786, @lebedev.ri wrote:

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

Hi @lebedev.ri, the first profile is with the vanilla llvm.

(cc @spatel, re rG0aa75fb12faa04e07ba1a6e334605357b6a159c9)

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

In D94355#2492394, @lebedev.ri wrote:

Then, tangential question: shouldn't this simply be a separate transformation, to turn non-PIC-friendly (non-relative) LUT's into relative LUT's?

Thanks for the feedback. Do you suggest to implement this as an another optimization after SwitchToLookup in SimplifyCFG?

I'm not really sure where it would fit best. Assuming it doesn't require changing CFG
this might even be eligible for InstCombine, but adding it into simplifycfg is a safe bet i guess.

@aaron.ballman thank you for taking a look!
@NoQ thank you for commenting.

In D66571#2491979, @craig.topper wrote:

I don't think I ever did anything more with this. @lebedev.ri did you want to pick this up?

Then, tangential question: shouldn't this simply be a separate transformation, to turn non-PIC-friendly (non-relative) LUT's into relative LUT's?

In D94014#2490627, @spatel wrote:

In D94014#2490458, @nlopes wrote:

Another way is to use assume to encode the precondition if you prefer: https://alive2.llvm.org/ce/z/__szVL

In D94014#2490431, @lebedev.ri wrote:

https://alive2.llvm.org/ce/z/u5hCcz

Thanks for the improved examples!

In D94014#2490334, @spatel wrote:

Anyone see problems with this Alive2 implementation using count-leading-*?
https://alive2.llvm.org/ce/z/SWxadd

I also manually entered all of the i4 regression tests with fixed constants in Alive1 (rise4fun), and they appear to be correct as shown in the test diffs.

Sorry for the delay.

In D93822#2474475, @dblaikie wrote:

In D93822#2474355, @lebedev.ri wrote:

Thank you for taking a look!

In D93822#2474236, @dblaikie wrote:

I would guess it doesn't meet the on-by-default bar we would prefer for Clang,

Yeah, based on my initial observations, it's pretty chatty.

but might still be OK.

I'm open to input here, but it would be good to have this at least in -Wextra (-Weverything always being a last resort option).

Yeah, not sure how @rsmith, @rtrieu, or @aaron.ballman feel about what the bar for warnings is these days - used to be a bit more hardcore about the "if it can't be on by default it shouldn't be in clang" than we are these days, I think. I'd guess -Wextra might be suitable.

In D94367#2489090, @fhahn wrote:

Thanks for putting up the patch!

I think the code here and D94366 could be simplified if we would introduce 2 codegen options to distinguish the differences between C and C++:

• functions-must-progress=true/false: C++ behavior >= c++11; all threads must make forward progress, we should be able to mark all functions as mustprogress and there should be no need to mark the individual loops as mustprogress.
• loops-must-progress=all/none/c11: all -> all loops are marked mustprogress, none -> no loops are marked, c11 -> all loops that do not match the C11 escape hatch are marked mustprogress.

Now

• c++11 and above, functions-must-progress=true, loops-must-progress=none,
• c11 and above functions-must-progress=false, loops-must-progress=c11,
• -ffinite-loops -> loops-must-progress=all,
• -fnofinite-loops -> , functions-must-progress=false, loops-must-progress=false

In CodeGen, we always add mustprogress to functions exactly if functions-must-progress=true. For loops, we could have a helper shouldMarkLoopAsMustProgress(bool C11Exception), which returns true depending on loops-must-progress and C11Exceptions, which the caller sets to true if the loop condition is one that is allowed for infinite loops in C11.

This should allow us to remove CGF::FnIsMustProgress and the code to update it; now either all functions have mustprogress or none have, depending on functions-must-progress. We still need CGLoopInfo::MustProgress, but we only need to update it depending on loops-must-progress,

I think this would overall simplify the handling in Codegen, but it's very possible that I am missing something from earlier discussions. I am also adding a few additional people who may have additional thought.

One things that's slightly odd with the proposal is that for C++, loops-must-progress would be none, which has the potential to be a bit confusing. We could make this clear in the documentation or just set it to all, although as I mentioned in the beginning, that should not really be necessary. It might be helpful/necessary if loops inlined from C++ function inside C functions should keep their mustprogress property.

What specifically is having problems after that transformation?

Why must values have pointer types?

In D89697#2484919, @spatel wrote:

In D89697#2483676, @yubing wrote:

No regression appeared in our internal testcases.
It seems the transform is correct, have you verified it with alive-tv?

I was curious to see if I could model it:
https://alive2.llvm.org/ce/z/RXcYY9

Converting #x4f800000 (4294967296) to uint32_t is poison, not 0 though. Am I reading the Alive output correctly? (cc @lebedev.ri @aqjune @nlopes @nikic )

This seems to be missing test coverage.

In D93888#2483368, @wecing wrote:

For NewGVN and IPSCCP PredicateInfo would have to handle it, and I'm pretty sure we want to avoid that.

@nikic Would you mind explaining why? If there is a good reason we cannot do this in NewGVN then D93850 might be the best we could do.

Fixed by 29ca7d5a1ad968c371124b8d82edd8d91eee7b4f, thanks!

I do not understand the example in https://bugs.llvm.org/show_bug.cgi?id=48529
By "intent", are you saying that the canonicalization of commutative ops
is causing the code to be miscompiled, leading to different observable side-effects?

In rG2d07414ee5f74a09fb89723b4a9bb0818bdc2e18#968162, @uabelho wrote:

Hi!

Hi,

Looks good to me, thanks.

I'm being dense here, sorry. I thought benchmark is usually built with -std=c++14,
but as per llvm-project/llvm/utils/benchmark/CMakeLists.txt, that is not the case.
So i'll just reland this.