LGTM

I added a similar fold here:
0399473de886

Abandoning - looks like this will need a target hook of some kind along with the more general transform in D128123.

LGTM

LGTM - noted a close call in x86 tests, but that can be addressed if we find real regressions.

I committed the tests with baseline results, so we have those in place independently of the code change. I then updated the tests to provide a bit more coverage. Please rebase/update after:
feb4b336acc71

I put up D128123 as the potential better/final patch, so we can see the current test diffs.

In D127903#3594030, @emgullufsen wrote:

Fixed one more incorrect use of capturing in CHECK line.

In D128080#3594019, @spatel wrote:

I also tracked down where this happens in x86 codegen:
https://github.com/llvm/llvm-project/blob/cd64a427efa0baaf1bb7ae624d4301908afc07f7/llvm/lib/Target/X86/X86InstrCompiler.td#L1535

So this fold and the InstCombine that I copied from are actually over-constrained. We don't need a low-bit-mask constant:
https://alive2.llvm.org/ce/z/OUm6N_

In D128080#3594009, @deadalnix wrote:

It doesn't looks like it is helping D127115, but it looks useful on its own.

The uploaded diff is against the previous revision of this patch. Please rebase so it is against (and can be applied cleanly to) the main branch.

Thanks - let's give this a try.
It seems like it can help IR analysis, and there are potential wins for codegen too. If we find perf regressions that are not easy to reverse, then we can revert.

In D127854#3591928, @Allen wrote:

rebase

LGTM

LGTM - see inline for some possible minor improvements.

LGTM

LGTM - thanks for the thorough tests!
See inline for some minor cleanups.

I don't know enough to visualize the GEP corner-cases or the interaction with LICM, but I commented on general improvements to the patch.

In D127392#3581102, @bipmis wrote:

https://godbolt.org/z/Ee4cbf1PG
Similar situation can be seen with the unroller as well as in the example below:
https://godbolt.org/z/dWo8n3Yz8

In D127801#3585983, @bcl5980 wrote:

Actually even if the instruction number are the same, we still prefer shift+and because cmp port is less than shift for most modern CPU I think.

In D127801#3584097, @bcl5980 wrote:

We need to assume X < BitWidth https://alive2.llvm.org/ce/z/-GjVsu

LGTM - as noted in 21d7c3bcc646f5db73b, this pattern is probably not common because it's not the canonical IR.

LGTM

I'd like to see some form of load combining in IR, but we need to be very careful about how that is implemented.

I don't like increasing risk by combining what are really 2 independent changes into 1 patch.
Please go back to the earlier revision that only adds the new fold. We can ease the vector restrictions as a second step (I haven't seen any evidence that those cases matter, but it opens up more possibilities to miscompile in ways that are very hard to detect).

In D123408#3572528, @mstorsjo wrote:

afa192cfb604

That commit causes breakage for me - one file which used to compile in around a second now hangs (doesn't complete in many minutes at least).

To reproduce, download https://martin.st/temp/remap-preproc.c and try to compile it with clang -target x86_64-w64-mingw32 -w -c -O2 remap-preproc.c.

Note that we likely have FMF propagation bugs in this code based on discussion/examples in D127275. If you can put a TODO comment on this, that would be helpful. We'll probably need to add more tests with mixed FMF to demonstrate the bugs.

Note: I think we should still do the narrowing transform that I suggested in D123408, but that's independent of this fix. That narrowing commit ran into its own infinite loop due to a conflict matching constant expressions, but I can fix that.

In D123408#3571242, @syzaara wrote:

Oh nevermind, please ignore above comment, I see what you are saying now! That we can add a transformation which can convert:

In D123408#3570605, @syzaara wrote:

@spatel Could you please provide some feedback about this change to InstCombine.

If I'm seeing it correctly, this will alter D126591 or possibly make it unnecessary. I recommend implementing the symmetric TODO pattern for this patch as the next patch, and then we can see what remains.

In D126638#3569452, @nikic wrote:

For what input is that? As far as I can tell, AArch64 never uses despeculation, and ARM uses a libcall legalization where it makes no difference. I'm trying this one:

LGTM

LGTM

LGTM - it may be worth looking at the related folds to see if there's some generalization that we missed (see inline comment about the other TODO comment in the test file).

In D126617#3565340, @bcl5980 wrote:

1. Use m_Power2 to match C1
2. Remove condition Log2(C1) < Log2(C3)+C2

https://alive2.llvm.org/ce/z/iwd4ba
This is needed for correctness, so LGTM.

It would be good to refactor foldSelectIntoOp() with a lambda as a preliminary cleanup, so we don't have to duplicate the code.
But this is an improvement even without that, so LGTM.

If this is inverting the transform from D126774, do we need 'nsz' to avoid miscompiling -0.0?
https://alive2.llvm.org/ce/z/4IMKLM

For reference, this was the Alive2 from the original review ( D117365 ):
https://alive2.llvm.org/ce/z/tFAcZt

In D126190#3563544, @bsmith wrote:

This has turned out to be far more complex to do safely than originally thought, given I don't as of yet have a compelling example where this provides a significant benefit I will abandon this for now.

In D126774#3563036, @david-arm wrote:

• Added more pattern matches for MVE targets so we detect splats of +0.0 as well as -0.0.

Adjusted code to allow non-uniform (non-splat) vector constants on the shifted (inner) constant.

In D126617#3559375, @bcl5980 wrote:

1. the demanded bits fix. I think it is a safe and independent change.

IIUC, the problem/fix (and it would be good to put something like this in the updated commit message for easier reference):
The previous revision/commit did not check one-use of an intermediate value that this transform re-uses. When that value has another use, an existing transform will try to invert the transform here. By adding one-use checks, we avoid the infinite loops seen with the earlier commit.

In D126889#3559904, @foad wrote:

No objection from me, and I do maintain downstream forks. Can you provide a simple one-line command that I can run to make the equivalent changes in my downstream?