-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Lower BinOp::Cmp to llvm.{s,u}cmp.* intrinsics #133984
base: master
Are you sure you want to change the base?
Conversation
LLVM 19 doesn't have all the optimization support for these intrinsics yet. |
Should the check be on LLVM 20? |
This comment has been minimized.
This comment has been minimized.
I'm excited to do this, but if nikic says it's too early, that probably means it shouldn't be on by default yet. Maybe make this PR adding a -Z flag for it, so people can play with it and test out how close it is? (Then in the future once it's fully ready we can remove that flag again) |
Yes, that's fine. You'll have to test locally against LLVM 20 though to make sure codegen tests don't break. |
Lowers `mir::BinOp::Cmp` (`three_way_compare` intrinsic) to the corresponding LLVM `llvm.{s,u}cmp.i8.*` intrinsics, added in LLVM 19.
@rustbot ready I didn't add the unstable flag since LLVM 20 already tries to lower the manual icmp pattern to the intrinsic so doing it ourselves shouldn't change much. |
Given that https://rust-lang.zulipchat.com/#narrow/channel/187780-t-compiler.2Fwg-llvm/topic/LLVM.2020/near/495752627 is talking about landing LLVM 20 in nightly in just a couple weeks, should we wait for that before doing this PR? I don't know how much we get from having it now in LLVM 19, when the LLVM 20 versions of the tests won't run in bors. |
Lowers
mir::BinOp::Cmp
(three_way_compare
intrinsic) to the corresponding LLVMllvm.{s,u}cmp.i8.*
intrinsics.These are the intrinsics mentioned in #118310, which are now available in LLVM 19.
I couldn't find any follow-up PRs/discussions about this, please let me know if I missed something.
r? @scottmcm