Implemente overflowing_sh* with new unchecked_sh* intrinsics by TimNN · Pull Request #40521 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation18 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Also update some 128 bit builtins to not rely on the constant evaluator to avoid checked operations.

Fixes #40508.

cc @nagisa, @alexcrichton

Note: I still have a build running to see if the 128 bit changes worked (unoptimized builds take forever to compile), however at least the overflowing builtins no longer reference core::panicking::panic.

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -311,6 +311,8 @@ pub fn trans_intrinsic_call<'a, 'tcx>(bcx: &Builder<'a, 'tcx>,
} else {
bcx.urem(llargs[0], llargs[1])
},
"unchecked_shl" => bcx.shl(llargs[0], llargs[1]),
"unchecked_shr" => bcx.lshr(llargs[0], llargs[1]),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be either ashr when types are signed or lshr when types are unsigned.

See for example the rustc_trans::common::build_unchecked_rshift (you can’t reuse it sadly, because it tries to ensure the validity of RHS).

#[cfg(not(stage0))]
pub fn overflowing_shl(self, rhs: u32) -> (Self, bool) {
(unsafe {
intrinsics::unchecked_shl(self, (rhs & ($BITS - 1)) as $SelfT)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you’re already working around this code, it would be nice if you used this to implement wrapping_shl and used wrapping_shl to implement overflowing_shl instead of the inverse that we have today.

@@ -83,8 +83,8 @@ pub mod reimpls {
macro_rules! lshr {
($a: expr, b:expr,b: expr, b:expr,ty:ty) => {{
let (a, b) = ($a, $b);
let bits = (::core::mem::size_of::<$ty>() * 8) as $ty;
let half_bits = bits >> 1;
let bits = ::core::mem::size_of::<$ty>().overflowing_mul(8) as $ty;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/overflowing/wrapping/?

@bors r=nagisa delegate=nagisa

✌️ @nagisa can now approve this pull request

📌 Commit ceccedd has been approved by nagisa

I missed one of the i128 macros :(

Maybe leave this PR until tomorrow, I'll have a full unoptimized ARM build done by then to verify that actually all occurrences of panic are gone.

Good news! readelf no longer shows any references to panic. However run-pass/issue-28950.rs hanges somewhere, but I think that is a separate issue, so this is probably good to go to bors.

(The travis failure is about the mac bots being... broken.)

Do you want me to squash the commits?