Skip to content

[InstCombine] Regression in and of masked comparison fold #110919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nikic opened this issue Oct 2, 2024 · 1 comment · Fixed by #112704
Closed

[InstCombine] Regression in and of masked comparison fold #110919

nikic opened this issue Oct 2, 2024 · 1 comment · Fixed by #112704

Comments

@nikic
Copy link
Contributor

nikic commented Oct 2, 2024

From rust-lang/rust#131162:

const MASK: u8 = 1;

#[no_mangle]
pub fn test1(a1: u8, a2: u8) -> bool {
    (a1 & !MASK) == (a2 & !MASK) && (a1 & MASK) == (a2 & MASK)
}

https://llvm.godbolt.org/z/Pasonhe9d:

define i1 @test(i8 %a1, i8 %a2) {
  %xor = xor i8 %a1, %a2
  %cmp = icmp ult i8 %xor, 2
  %lobit = trunc i8 %xor to i1
  %lobit.inv = xor i1 %lobit, true
  %and = and i1 %cmp, %lobit.inv
  ret i1 %and
}

LLVM 18:

define i1 @test(i8 %a1, i8 %a2) {
  %and = icmp eq i8 %a1, %a2
  ret i1 %and
}

LLVM 19:

define i1 @test(i8 %a1, i8 %a2) {
  %xor = xor i8 %a1, %a2
  %cmp = icmp ult i8 %xor, 2
  %lobit = trunc i8 %xor to i1
  %lobit.inv = xor i1 %lobit, true
  %and = and i1 %cmp, %lobit.inv
  ret i1 %and
}

I believe this is fallout from removal of the trunc i1 canonicalization in #84628.

@nikic nikic changed the title [InstCombine] Regression and of masked comparison fold [InstCombine] Regression in and of masked comparison fold Oct 2, 2024
@nikic
Copy link
Contributor Author

nikic commented Oct 16, 2024

Inverse case: https://llvm.godbolt.org/z/qsq8cj9eK

Adding handling for this to foldEqOfParts() is not as straightforward as I'd like because it moves away from "and/or of icmps" and we're quite geared towards that right now. Like, it will no longer be automatically invoked for logical and/or, reassociated and/or, etc. This should probably be cleaned up first.

@nikic nikic self-assigned this Oct 16, 2024
nikic added a commit that referenced this issue Oct 17, 2024
nikic added a commit to nikic/llvm-project that referenced this issue Oct 17, 2024
Equality/inequality of the low bit can be represented by
`(trunc (xor x, y) to i1)`, possibly with an extra not. We have
to handle this in the eq-of-parts fold now that we no longer
canonicalize this to a masked icmp.

Proofs: https://alive2.llvm.org/ce/z/qidkzq

Fixes llvm#110919.
nikic added a commit to nikic/llvm-project that referenced this issue Nov 25, 2024
Equality/inequality of the low bit can be represented by
`(trunc (xor x, y) to i1)`, possibly with an extra not. We have
to handle this in the eq-of-parts fold now that we no longer
canonicalize this to a masked icmp.

Proofs: https://alive2.llvm.org/ce/z/qidkzq

Fixes llvm#110919.
nikic added a commit that referenced this issue Nov 25, 2024
Equality/inequality of the low bit can be represented by `(trunc (xor x,
y) to i1)`, possibly with an extra not. We have to handle this in the
eq-of-parts fold now that we no longer canonicalize this to a masked
icmp.

Proofs: https://alive2.llvm.org/ce/z/qidkzq

Fixes #110919.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant