De-LLVM the unchecked shifts [MCP#693] #123226
Conversation
rustbot
added
S-waiting-on-review
|
Note that I did not change the traits for llvm/gcc to implement in this. Updating the intrinsics -- as rust concepts -- to match what we want them to be definitely makes sense to me. I'm less sure about the |
This comment has been minimized.
This comment has been minimized.
|
This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is just one part of the MCP, but it's the one that IMHO removes the most noise from the standard library code. Seems net simpler this way, since MIR already supported heterogeneous shifts anyway, and thus it's not more work for backends than before.
This comment confusingly disappeared, but for CTFE/Miri it is true. The MIR binops support different types for LHS and RHS (and these intrinsics lower to MIR binops). |
I was actually stuck on figuring the shifts out. Since you can have integer shifts with any integer rhs, backends will need to cast, no matter what, so I couldn't figure out how to make everything better for everyone. I have all the other changes ready (I think), so I'll open a PR once we merge this (I'll try to review soon). |
There was a problem hiding this comment.
I have some small nits and am somewhat confused why we want to make intrinsics generic wrt lhs. But LGTM on the general approach of moving things from std to the backend.
This does not particularly help us at my dayjob (we would like bx.shl to take rhs which is u32, i.e. we would like to have a different build_shift_expr_rhs), but I do like this change anyway. 😅
| #[rustc_const_stable(feature = "const_int_unchecked", since = "1.40.0")] | ||
| #[rustc_nounwind] | ||
| pub fn unchecked_shl<T: Copy>(x: T, y: T) -> T; | ||
| pub fn unchecked_shl<T: Copy, U: Copy>(x: T, y: U) -> T; |
There was a problem hiding this comment.
Is there a reason to make shift amount generic? Why not just y: u32?
There was a problem hiding this comment.
Making the shift amount always u32 was actually what I did first.
However, MIR actually supports arbitrary heterogeneous shifts, even though for the unchecked once those weren't accessible. So when the RHS was always u32, that blew up a whole bunch of tests -- many wouldn't even compile, with -1 not being a valid u32.
So supporting mixed types here is helpful in that we can still write tests that exercise those codepaths in CTFE and Codegen. And it doesn't make the lowering to MIR any harder, nor add any extra work to what the backends already needed to handle. It also turned out convenient in that the align_offset method actually ends up using unchecked_shl<usize, usize> with this this PR, though once you change cttz_nonzero it'll also end up at unchecked_shl<usize, u32>, I think.
There was a problem hiding this comment.
A bit weird (given that we don't publicly expose this), but given this does not make backend more complex, ig this is fine.
| @@ -27,7 +27,7 @@ const SHL_U128: u128 = unsafe { intrinsics::unchecked_shl(5_u128, 128) }; | |||
|
|
|||
| const SHL_I8: i8 = unsafe { intrinsics::unchecked_shl(5_i8, 8) }; | |||
| //~^ ERROR evaluation of constant value failed | |||
| const SHL_I16: i16 = unsafe { intrinsics::unchecked_shl(5_16, 16) }; | |||
| const SHL_I16: i16 = unsafe { intrinsics::unchecked_shl(5_i16, 16) }; | |||
There was a problem hiding this comment.
Yeah, this is a fun typo. So much so that I opened a clippy issue for a lint about it *looks* wow, 6 years ago now rust-lang/rust-clippy#3091
rustbot
added
S-waiting-on-author
Co-authored-by: Waffle Maybe <waffle.lapkin@gmail.com>
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
De-LLVM the unchecked shifts [MCP#693] This is just one part of the MCP (rust-lang/compiler-team#693), but it's the one that IMHO removes the most noise from the standard library code. Seems net simpler this way, since MIR already supported heterogeneous shifts anyway, and thus it's not more work for backends than before. r? WaffleLapkin
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#123198 (Add fn const BuildHasherDefault::new) - rust-lang#123226 (De-LLVM the unchecked shifts [MCP#693]) - rust-lang#123302 (Make sure to insert `Sized` bound first into clauses list) - rust-lang#123348 (rustdoc: add a couple of regression tests) - rust-lang#123362 (Check that nested statics in thread locals are duplicated per thread.) - rust-lang#123368 (CFI: Support non-general coroutines) - rust-lang#123375 (rustdoc: synthetic auto trait impls: accept unresolved region vars for now) - rust-lang#123378 (Update sysinfo to 0.30.8) Failed merges: - rust-lang#123349 (Fix capture analysis for by-move closure bodies) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123226 - scottmcm:u32-shifts, r=WaffleLapkin De-LLVM the unchecked shifts [MCP#693] This is just one part of the MCP (rust-lang/compiler-team#693), but it's the one that IMHO removes the most noise from the standard library code. Seems net simpler this way, since MIR already supported heterogeneous shifts anyway, and thus it's not more work for backends than before. r? WaffleLapkin
…tmcm,RalfJung,antoyo Dellvmize some intrinsics (use `u32` instead of `Self` in some integer intrinsics) This implements rust-lang/compiler-team#693 minus what was implemented in rust-lang#123226. Note: I decided to _not_ change `shl`/... builder methods, as it just doesn't seem worth it. r? `@scottmcm`
…tmcm,RalfJung,antoyo Dellvmize some intrinsics (use `u32` instead of `Self` in some integer intrinsics) This implements rust-lang/compiler-team#693 minus what was implemented in rust-lang#123226. Note: I decided to _not_ change `shl`/... builder methods, as it just doesn't seem worth it. r? ``@scottmcm``
Rollup merge of rust-lang#124003 - WaffleLapkin:dellvmization, r=scottmcm,RalfJung,antoyo Dellvmize some intrinsics (use `u32` instead of `Self` in some integer intrinsics) This implements rust-lang/compiler-team#693 minus what was implemented in rust-lang#123226. Note: I decided to _not_ change `shl`/... builder methods, as it just doesn't seem worth it. r? ``@scottmcm``
…tmcm,RalfJung,antoyo Dellvmize some intrinsics (use `u32` instead of `Self` in some integer intrinsics) This implements rust-lang/compiler-team#693 minus what was implemented in rust-lang#123226. Note: I decided to _not_ change `shl`/... builder methods, as it just doesn't seem worth it. r? ``@scottmcm``
…tmcm,RalfJung,antoyo Dellvmize some intrinsics (use `u32` instead of `Self` in some integer intrinsics) This implements rust-lang/compiler-team#693 minus what was implemented in rust-lang#123226. Note: I decided to _not_ change `shl`/... builder methods, as it just doesn't seem worth it. r? ``@scottmcm``
This is just one part of the MCP (rust-lang/compiler-team#693), but it's the one that IMHO removes the most noise from the standard library code.
Seems net simpler this way, since MIR already supported heterogeneous shifts anyway, and thus it's not more work for backends than before.
r? WaffleLapkin