Skip to content
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

(trunc nuw A) == (trunc nuw B) should not take multiple xors [InstCombine] #133344

Closed
scottmcm opened this issue Mar 27, 2025 · 0 comments · Fixed by #133368
Closed

(trunc nuw A) == (trunc nuw B) should not take multiple xors [InstCombine] #133344

scottmcm opened this issue Mar 27, 2025 · 0 comments · Fixed by #133368

Comments

@scottmcm
Copy link

scottmcm commented Mar 27, 2025

Take this input IR:

define noundef i1 @src(i8 noundef %a, i8 noundef %b) local_unnamed_addr {
  %at = trunc nuw i8 %a to i1
  %bt = trunc nuw i8 %b to i1
  %eq = icmp eq i1 %at, %bt
  ret i1 %eq
}

(That's the kind of thing you might see from looking at bools in Options or unions in Rust.)

Today, it "optimizes" to a lossy truncation and a couple of xors:

define noundef i1 @src(i8 noundef %a, i8 noundef %b) local_unnamed_addr #0 {
  %1 = xor i8 %b, %a
  %2 = trunc i8 %1 to i1
  %eq = xor i1 %2, true
  ret i1 %eq
}

Now, if the nuws weren't there that would make sense, but with the nuws it would be much better to simplify it to

define noundef i1 @tgt(i8 noundef %a, i8 noundef %b) local_unnamed_addr {
  %eq = icmp eq i8 %a, %b
  ret i1 %eq
}

instead. Alive2 proof: https://alive2.llvm.org/ce/z/X3Uh23

Notably, that does happen already if you truncate to something that's not i1, like https://alive2.llvm.org/ce/z/H-QrGZ

define noundef i1 @src(i8 noundef %a, i8 noundef %b) local_unnamed_addr {
  %at = trunc nuw i8 %a to i2
  %bt = trunc nuw i8 %b to i2
  %eq = icmp eq i2 %at, %bt
  ret i1 %eq
}

So hopefully this is just a matter of disabling an i1 special case in InstCombine when nuw is there.


For completeness, looks like this simplification is also legal when both sides are nsw: https://alive2.llvm.org/ce/z/vRMFDD

In mixed cases (one nuw and one nsw) this is NOT applicable, however: https://alive2.llvm.org/ce/z/kUl-ZL


cc @nikic -- I think this might be part of the enduring problem of Option<bool>::eq being terrible you've mentioned before.

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.

4 participants