In D144274#4186866, @nikic wrote:

Hm, it looks like this ended up causing non-trivial codegen changes: https://llvm-compile-time-tracker.com/compare.php?from=772aa05452f8ff90a47168e6801cda2acb5a1873&to=43ae4b62b2671cf73e691c0b53324cd39405cd51&stat=size-text So we're not preserving previous behavior in some significant way(s).

LGTM

LGTM

In D145299#4169855, @nikic wrote:

In D145299#4169706, @spatel wrote:

In D145299#4169702, @RKSimon wrote:

What is preventing is from performing this in InstCombine? I don't think this pattern will emerge in SelectionDAG

I haven't found a way to avoid a poison shift in IR without doing a cmp+select or umax yet. I think we're relying on the x86-specific behavior of masking the shift amount to make that part of the logic disappear in this patch.

Well, we could also do what is being proposed here and mask the shift amount: https://alive2.llvm.org/ce/z/FD_9Sh On x86, that happens to be free. I'm not sure this makes sense as an IR canonicalization though.

In D145299#4169799, @goldstein.w.n wrote:

In D145299#4169706, @spatel wrote:

In D145299#4169702, @RKSimon wrote:

What is preventing is from performing this in InstCombine? I don't think this pattern will emerge in SelectionDAG

I haven't found a way to avoid a poison shift in IR without doing a cmp+select or umax yet. I think we're relying on the x86-specific behavior of masking the shift amount to make that part of the logic disappear in this patch.

The IR is:

%2 = add i32 %0, -1
%3 = tail call i32 @llvm.ctlz.i32(i32 %2, i1 false), !range !5
%4 = sub nuw nsw i32 32, %3
%5 = shl nuw i32 1, %4
%6 = icmp ugt i32 %0, 1
%7 = select i1 %6, i32 %5, i32 1
ret i32 %7

The poison shift is if %3 is zero?

LGTM

In D145299#4169702, @RKSimon wrote:

What is preventing is from performing this in InstCombine? I don't think this pattern will emerge in SelectionDAG

As noted in previous comments, it's hard to reliably pattern-match a sequence that is this long. We are missing several potential canonicalizations in IR, and I see at least one possible variant that is shorter in IR:
https://alive2.llvm.org/ce/z/CkQ433

In D144274#4167818, @tschuett wrote:

It is odd to me that instcombine has two modes: with and without loopinfo. Wouldn't completely removing loopinfo make instcombine better testable, hermetic, and predictable?

Patch updated:
Add a RUN line to the instcombine test to show with and without loop info test diff.

In D144274#4167715, @fhahn wrote:

This should probably have an instcombine test that shows the difference between instcombine<use-loop-info> and plain instcombine?

I didn't follow the reviews of D142473 and D144963 while they were in progress, but this is the logical conclusion to the set. LGTM.

This looks close to me, but see inline comments on the tests, and please pre-commit those with the baseline CHECK lines, so we'll just show test diffs in this patch.

Ping.

Patch updated:

1. Match any pair of no-wrap subtracts.
2. But only propagate "nsw" with restrictions.

LGTM

Patch updated:
Add restriction to freeze creation for unsigned ops because they are easier to determine as safe.

This is adding a lot of code to a function that's already too big. A helper function will make it easier to read.

Patch updated:
Added TODO comment to allow this for any binop (currently bails out on div/rem/shift).

LGTM - thanks!
See inline comments for a small adjustment to the tests.

Patch updated:
When necessary, freeze the select condition value to preserve the optimization.

In D144671#4154013, @efriedma wrote:

Alternatively, if you freeze the condition of the select, the transform is legal in all cases.

We should have 2 more tests: (1) extra use of the sub and (2) extra use of the and.
As noted earlier, this patch should not require a "m_OneUse" limitation. I realize that it looks like a regression for the existing test, but that should be ok when viewed globally: we are reducing to an equality compare, and GVN, CVP, or some other pass will reduce that to the optimal form. It's just lucky (or unlucky) that the min/max folds added with D144606 are able to reduce it all within InstCombine.

In D144777#4153685, @junaire wrote:

Hi @spatel, thanks for your comments, I updated the tests according to your
suggestions. However, everything stop folding after I use div instructions to
each oprands of and instruction.

Can you take a look? Is this because I missed something? Or my fold pattern is wrong.

Patch updated:

1. Use lambda and m_c_Binop to reduce code duplication (I tried a few variations of juggling the conditions - this was the best I could find).
2. Added a pair of negative tests with mismatched demand/shuffle mask.

In D142803#4149087, @bcl5980 wrote:

In D142803#4148486, @nikic wrote:

I'm concerned about the caching here. It looks like you reuse one LogicCombiner instance for a basic block. However, isn't it possible for some of the instructions that have been inserted into LogicalOpNodes to be deleted, in which case the map may contain dangling pointers. If the pointer is reused by a newly allocated instruction, the cached information will be incorrect.

The main reason for caching is saving compile time. The new patch will remove all the instructions already inserted into the caches and I think functional it works now.
@nikic @spatel if possible can we use the llvm-compile-time-track to test how much compile time increase if we enable the LogicCombiner for every single instruction?

LGTM

LGTM

Patch updated:
Changed instcombine pipeline invocations to not use loop info unless it is already available, so this is effectively NFC.

I need LeafValues to access value by index. It looks SmallSetVector can't do that.

Please rename the test file as a preliminary step, so we will again show diffs in this patch. We also need to add negative tests to show current limitations and also that the combining is not making wrong logic reductions.

In D144451#4142962, @RKSimon wrote:

Incidently, I'm guessing the vector equivalent code will be pretty poor as well when a umax op isn't available (v2i64 pre-AVX512 etc).

This seems close to ready to me. It would be great if other reviewers have a look too. :)

This doesn't handle i8 and i16 the same way because truncates get in the way, right? Would it be better to match this as "umax(x, 1)" (before it becomes x86-specific instructions)? Or could we peek through a truncate and still generate the adc for i8/i16?

Patch updated:
Reduced the impliesPoison() logic as suggested.

In D144493#4141817, @nikic wrote:

I like the approach, but I'm not sure you're checking the right poison implications. My first thought would be that we need impliesPoison(V, Op). We are replacing the select with V, so what we want to establish is that if V is poison, the select would be poison anyway because Op is also implied poison.

In D143593#4135928, @ManuelJBrito wrote:

In D143593#4130108, @spatel wrote:

What if we squash any cast of freeze(poison) into the freeze itself as an addition to getUndefReplacement():
https://alive2.llvm.org/ce/z/o7J2JS

That would give us a far-fetched improvement on something like this:
https://alive2.llvm.org/ce/z/hC_Gws
...so that could be an independent patch before the shuffle hack.

I think this would work great! The main hurdle is that getUndefReplacement's users expect a Constant.
In particular Constant::replaceUndefsWith, so I would have to change the return type of getUndefReplacement to Value and also change Constant::replaceUndefsWith to a take a Value as a Replacement.
I'm not sure which option is more desirable, the current patch or your suggestion.

I don't know anything about ExecutionEngine - adding potential reviewers based on the most recent git log entries for this directory.

In D142803#4137888, @bcl5980 wrote:

I was curious if enabling this for all logic ops would cause any compile-time regressions, but it seems like it has almost no cost:
https://llvm-compile-time-tracker.com/compare.php?from=0e90cd7551f2d0b151f7406e8f3848ec54e650bf&to=ae505cb2a674ac4c240c94a74fc04ee274321697&stat=instructions:u

Based on the result, can I just enable it for all logical operations by default and remove the option?

We should probably view this as a missed canonicalization in IR first:
https://alive2.llvm.org/ce/z/D2Aph4 (that should also work with eq predicates)