In D113970#3151642, @OmerAviram wrote:

In D113970#3151343, @spatel wrote:

Do we expect all vector types/targets to bypass this in some way? If yes, it's worth adding a comment to the patch description and/or code. If no, can we find a test that shows an asm difference?

What exactly do you mean by bypass it? This addition to haveNoCommonBitsSet works for vector types as well. Do you mean add another lit test for vector types?

LGTM

LGTM

Do we expect all vector types/targets to bypass this in some way? If yes, it's worth adding a comment to the patch description and/or code. If no, can we find a test that shows an asm difference?

As discussed in D114462, I noticed a pair of missing 'xor' simplifications based on these patterns, so I added those with:
892648b18a8c
b326c058146f

In D114462#3149477, @rampitec wrote:

LGTM.

Thanks Sanjay! This really helps D113216 with -O3, but then not a more complex version of it which needs demorganing and reassociation:

; (~a & ~b & c) | ~(a | (b | c)) -> ~(a | b)
; (~a & ~b & c) | ~(b | (a | c)) -> ~(a | b)

define i32 @not_and_not_and_or_not_or_or(i32 %a, i32 %b, i32 %c) {
; CHECK-LABEL: @not_and_not_and_or_not_or_or(
; CHECK-NEXT:    [[OR3:%.*]] = or i32 [[B:%.*]], [[A:%.*]]
; CHECK-NEXT:    [[NOT2:%.*]] = xor i32 [[OR3]], -1
; CHECK-NEXT:    ret i32 [[NOT2]]
;
  %or1 = or i32 %b, %c
  %or2 = or i32 %or1, %a
  %not1 = xor i32 %or2, -1
  %nota = xor i32 %a, -1
  %notb = xor i32 %b, -1
  %and1 = and i32 %nota, %notb
  %and2 = and i32 %and1, %c
  %or4 = or i32 %and2, %not1
  call void @use(i32 %nota)
  call void @use(i32 %notb)
  ret i32 %or4
}

declare void @use(i32)

In this case reassociation is blocked by uses, so I was looking into extending LHS match in D113216 to catch (~a & ~b & c) in addition to (~(a | b) & c). I still need to look into that, but this is clearly LGTM.

Note: we miss the sibling with swapped and/or too, and it doesn't need a 'not' op, so it would clearly be best placed in instsimplify:
https://alive2.llvm.org/ce/z/7LbWeV

In D113179#3146480, @RKSimon wrote:

any more comments? @lebedev.ri @spatel

1. Please commit the baseline tests, so we see diffs here.
2. I expect that will show that the commuted variations are not testing what they claim to be testing (you'll need to add instructions to prevent commuting).
3. Please add tests with extra uses - since we're creating 2 instructions, the fold should be good as long as any one of the intermediate values can be eliminated (has one use).

You wrote that fdiv ran overnight without completing. I wonder if that opcode is slower than the others. For example, did fadd/fsub also timeout?

LGTM

Double-check that I'm getting the pattern right, but I don't think the 'or' side of this should have 6 instructions. There has to be a smarter way (!), but I worked this out on paper with a truth table:
https://alive2.llvm.org/ce/z/BP3ZZm

LGTM

Ping.
As I mentioned, I'd prefer not to try to add folds and invert an existing fold in one patch. I did add more instcombine tests for a follow-up patch that would use decomposeBitTestICmp.

LGTM - there may be more potential uses in functions like InstCombinerImpl::SimplifyUsingDistributiveLaws() or other reassociation folds.

Do you have enough commits/reviews now to request commit access? ( https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access )

In D26149#3130357, @Joshua-401 wrote:

Did you consider doing the load combine at IR level? I'm now trying to combine loads at IR level during the InstCombine process, so I'm wondering why you didn't do that.

In D113603#3131572, @hans wrote:

creduce finished, posted the reduced input here: https://bugs.chromium.org/p/chromium/issues/detail?id=1270222#c5

In D113603#3131023, @RKSimon wrote:

In D113603#3130973, @hans wrote:

This caused assertions in Chromium builds targeting Windows. See https://bugs.chromium.org/p/chromium/issues/detail?id=1270222#c1 for a reproducer.

I've reverted in 5be64d416481c59dba5faae5e8b8a6fecb578082 until it can be fixed.

I think https://bugs.llvm.org/show_bug.cgi?id=52504 is the same issue

LGTM

LGTM

Patch updated:
Added tests with i1 types and runs to exercise AVX512 targets. I'm not very familiar with those patterns, but it looks like that could use some follow-ups? Pre-AVX512 looks ideal to me.

Minor update from previous rev - use "X" and "Y" names for captured values since that matches the code comment.

Patch updated:

1. Spell out "BitWidth" in code comment and patch description for less confusion.
2. Use local SDValue names for matching to avoid swap risk in subsequent folds.
3. Create x86 shift node directly.
4. Rebase after 11522cfcad6b / D113426 - so all of the results in the "vselect-zero.ll" file are ideal now if I'm seeing it properly.

LGTM

LGTM

In D113442#3119977, @huihuiz wrote:

I have some concern when I checked with alive2, for fadd https://alive2.llvm.org/ce/z/UjAMM_ alive2 complaints mis-matched outputs.

I don't see any test diffs with 2 signed preds. Is there any value in adding a test like that to further exercise the range logic?

In D113132#3117026, @rampitec wrote:

With all that said it looks like these patterns have to be extracted into a separate function and called for both 'or' and 'and'. But I don't think this shall preclude this NFC and the next D113141 to wait for it, these are mostly orthogonal to me.

Patch updated:
Use general VectorType APIs to make the transform safe for scalable vectors. It is not possible to write a test for this currently -- and surrounding code would likely crash -- but this should work with matchers (m_Not) that are made more flexible in the future.