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
[WIP] Implement unchecked_disjoint_bitor
#124601
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Some changes occurred in cc @BoxyUwU This PR changes Stable MIR cc @oli-obk, @celinval, @ouz-a This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
Also, I think this PR will have to wait until the lowest supported LLVM is 18 or maybe even 19 (I can't figure out which version adds |
@@ -308,6 +308,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
BitAnd => ImmTy::from_uint(l & r, left.layout), | |||
BitXor => ImmTy::from_uint(l ^ r, left.layout), | |||
|
|||
BitOrDisjoint => { | |||
if l & r != 0 { | |||
todo!(); // Can't figure out what to do here based on the surrounding code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to throw_ub!
. To that end, you first need to add a new error variant to UndefinedBehaviorInfo
, and then use that here.
Make sure to also add a Miri testcase that checks that we detect the UB here.
@@ -1478,6 +1478,8 @@ pub enum BinOp { | |||
BitAnd, | |||
/// The `|` operator (bitwise or) | |||
BitOr, | |||
/// Like `BitOr` and `BitXor`, but UB if results don't match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Like `BitOr` and `BitXor`, but UB if results don't match. | |
/// Like `BitOr`, but UB if any bit is set both in the left and right operand (`left & right != 0`). |
Let's express the requirement directly, in a way that matches the name, rather than indirectly.
On older LLVM, it can simply compile to regular |
Not sure how I'd do that. Do we have something like |
The job Click to see the possible cause of the failure (guessed by this bot)
|
If you grep for LLVM_VERSION_GE in RustWrapper.cpp you can find existing examples. |
☔ The latest upstream changes (presumably #125331) made this pull request unmergeable. Please resolve the merge conflicts. |
ACP
AFAICT this doesn't compile right now (claims unrecognized intrinsic), and I don't exactly know why, because it did compile once already (or maybe there was a user error somehow). I would appreciate some help.
Beyond that this needs tests, non-intrinsic fns, and at least one todo resolved.