Skip to content

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

New issue

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

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

Already on GitHub? # to your account

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.

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

Successfully merging a pull request may close this issue.

4 participants