Skip to content

Commit

Permalink
Rollup merge of rust-lang#109475 - scottmcm:simpler-shifts, r=WaffleL…
Browse files Browse the repository at this point in the history 
…apkin

Simpler checked shifts in MIR building

Doing masking to check unsigned shift amounts is overcomplicated; just comparing the shift directly saves a statement and a temporary, as well as is much easier to read as a human.  And shifting by unsigned is the canonical case -- notably, all the library shifting methods (that don't support every type) take shift RHSs as `u32` -- so we might as well make that simpler since it's easy to do so.

This PR also changes *signed* shift amounts to `IntToInt` casts and then uses the same check as for unsigned.  The bit-masking is a nice trick, but for example LLVM actually canonicalizes it to an unsigned comparison anyway <https://rust.godbolt.org/z/8h59fMGT4> so I don't think it's worth the effort and the extra `Constant`.  (If MIR's `assert` was `assert_nz` then the masking might make sense, but when the `!=` uses another statement I think the comparison is better.)

To review, I suggest looking at rust-lang@2ee0468 first -- that's the interesting code change and has a MIR diff.

My favourite part of the diff:
```diff
-        _20 = BitAnd(_19, const 340282366920938463463374607431768211448_u128); // scope 0 at $DIR/shifts.rs:+2:34: +2:44
-        _21 = Ne(move _20, const 0_u128); // scope 0 at $DIR/shifts.rs:+2:34: +2:44
-        assert(!move _21, "attempt to shift right by `{}`, which would overflow", _19) -> [success: bb3, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+2:34: +2:44
+        _18 = Lt(_17, const 8_u128);     // scope 0 at $DIR/shifts.rs:+2:34: +2:44
+        assert(move _18, "attempt to shift right by `{}`, which would overflow", _17) -> [success: bb3, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+2:34: +2:44
```
  • Loading branch information
@matthiaskrgr
matthiaskrgr committed Mar 23, 2023
2 parents 80bfee1 + b537e6b commit 2603ef0
Show file tree
Hide file tree
Showing 6 changed files with 371 additions and 37 deletions.
56 changes: 33 additions & 23 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,41 +566,51 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Rvalue::Use(Operand::Move(val))
}
BinOp::Shl | BinOp::Shr if self.check_overflow && ty.is_integral() => {
// Consider that the shift overflows if `rhs < 0` or `rhs >= bits`.
// This can be encoded as a single operation as `(rhs & -bits) != 0`.
let (size, _) = ty.int_size_and_signed(self.tcx);
let bits = size.bits();
debug_assert!(bits.is_power_of_two());
let mask = !((bits - 1) as u128);

// For an unsigned RHS, the shift is in-range for `rhs < bits`.
// For a signed RHS, `IntToInt` cast to the equivalent unsigned
// type and do that same comparison. Because the type is the
// same size, there's no negative shift amount that ends up
// overlapping with valid ones, thus it catches negatives too.
let (lhs_size, _) = ty.int_size_and_signed(self.tcx);
let rhs_ty = rhs.ty(&self.local_decls, self.tcx);
let (rhs_size, _) = rhs_ty.int_size_and_signed(self.tcx);
let mask = Operand::const_from_scalar(

let (unsigned_rhs, unsigned_ty) = match rhs_ty.kind() {
ty::Uint(_) => (rhs.to_copy(), rhs_ty),
ty::Int(int_width) => {
let uint_ty = self.tcx.mk_mach_uint(int_width.to_unsigned());
let rhs_temp = self.temp(uint_ty, span);
self.cfg.push_assign(
block,
source_info,
rhs_temp,
Rvalue::Cast(CastKind::IntToInt, rhs.to_copy(), uint_ty),
);
(Operand::Move(rhs_temp), uint_ty)
}
_ => unreachable!("only integers are shiftable"),
};

// This can't overflow because the largest shiftable types are 128-bit,
// which fits in `u8`, the smallest possible `unsigned_ty`.
// (And `from_uint` will `bug!` if that's ever no longer true.)
let lhs_bits = Operand::const_from_scalar(
self.tcx,
rhs_ty,
Scalar::from_uint(rhs_size.truncate(mask), rhs_size),
unsigned_ty,
Scalar::from_uint(lhs_size.bits(), rhs_size),
span,
);

let outer_bits = self.temp(rhs_ty, span);
self.cfg.push_assign(
block,
source_info,
outer_bits,
Rvalue::BinaryOp(BinOp::BitAnd, Box::new((rhs.to_copy(), mask))),
);

let overflows = self.temp(bool_ty, span);
let zero = self.zero_literal(span, rhs_ty);
let inbounds = self.temp(bool_ty, span);
self.cfg.push_assign(
block,
source_info,
overflows,
Rvalue::BinaryOp(BinOp::Ne, Box::new((Operand::Move(outer_bits), zero))),
inbounds,
Rvalue::BinaryOp(BinOp::Lt, Box::new((unsigned_rhs, lhs_bits))),
);

let overflow_err = AssertKind::Overflow(op, lhs.to_copy(), rhs.to_copy());
block = self.assert(block, Operand::Move(overflows), false, overflow_err, span);
block = self.assert(block, Operand::Move(inbounds), true, overflow_err, span);
Rvalue::BinaryOp(op, Box::new((lhs, rhs)))
}
BinOp::Div | BinOp::Rem if ty.is_integral() => {
Expand Down
22 changes: 22 additions & 0 deletions compiler/rustc_type_ir/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,17 @@ impl IntTy {
_ => *self,
}
}

pub fn to_unsigned(self) -> UintTy {
match self {
IntTy::Isize => UintTy::Usize,
IntTy::I8 => UintTy::U8,
IntTy::I16 => UintTy::U16,
IntTy::I32 => UintTy::U32,
IntTy::I64 => UintTy::U64,
IntTy::I128 => UintTy::U128,
}
}
}

#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Debug)]
Expand Down Expand Up @@ -479,6 +490,17 @@ impl UintTy {
_ => *self,
}
}

pub fn to_signed(self) -> IntTy {
match self {
UintTy::Usize => IntTy::Isize,
UintTy::U8 => IntTy::I8,
UintTy::U16 => IntTy::I16,
UintTy::U32 => IntTy::I32,
UintTy::U64 => IntTy::I64,
UintTy::U128 => IntTy::I128,
}
}
}

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
Expand Down
20 changes: 20 additions & 0 deletions tests/mir-opt/building/shifts.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// compile-flags: -C debug-assertions=yes

// EMIT_MIR shifts.shift_signed.built.after.mir
fn shift_signed(small: i8, big: u128, a: i8, b: i32, c: i128) -> ([i8; 3], [u128; 3]) {
(
[small >> a, small >> b, small >> c],
[big << a, big << b, big << c],
)
}

// EMIT_MIR shifts.shift_unsigned.built.after.mir
fn shift_unsigned(small: u8, big: i128, a: u8, b: u32, c: u128) -> ([u8; 3], [i128; 3]) {
(
[small >> a, small >> b, small >> c],
[big << a, big << b, big << c],
)
}

fn main() {
}
147 changes: 147 additions & 0 deletions tests/mir-opt/building/shifts.shift_signed.built.after.mir
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// MIR for `shift_signed` after built

fn shift_signed(_1: i8, _2: u128, _3: i8, _4: i32, _5: i128) -> ([i8; 3], [u128; 3]) {
debug small => _1; // in scope 0 at $DIR/shifts.rs:+0:17: +0:22
debug big => _2; // in scope 0 at $DIR/shifts.rs:+0:28: +0:31
debug a => _3; // in scope 0 at $DIR/shifts.rs:+0:39: +0:40
debug b => _4; // in scope 0 at $DIR/shifts.rs:+0:46: +0:47
debug c => _5; // in scope 0 at $DIR/shifts.rs:+0:54: +0:55
let mut _0: ([i8; 3], [u128; 3]); // return place in scope 0 at $DIR/shifts.rs:+0:66: +0:86
let mut _6: [i8; 3]; // in scope 0 at $DIR/shifts.rs:+2:9: +2:45
let mut _7: i8; // in scope 0 at $DIR/shifts.rs:+2:10: +2:20
let mut _8: i8; // in scope 0 at $DIR/shifts.rs:+2:10: +2:15
let mut _9: i8; // in scope 0 at $DIR/shifts.rs:+2:19: +2:20
let mut _10: u8; // in scope 0 at $DIR/shifts.rs:+2:10: +2:20
let mut _11: bool; // in scope 0 at $DIR/shifts.rs:+2:10: +2:20
let mut _12: i8; // in scope 0 at $DIR/shifts.rs:+2:22: +2:32
let mut _13: i8; // in scope 0 at $DIR/shifts.rs:+2:22: +2:27
let mut _14: i32; // in scope 0 at $DIR/shifts.rs:+2:31: +2:32
let mut _15: u32; // in scope 0 at $DIR/shifts.rs:+2:22: +2:32
let mut _16: bool; // in scope 0 at $DIR/shifts.rs:+2:22: +2:32
let mut _17: i8; // in scope 0 at $DIR/shifts.rs:+2:34: +2:44
let mut _18: i8; // in scope 0 at $DIR/shifts.rs:+2:34: +2:39
let mut _19: i128; // in scope 0 at $DIR/shifts.rs:+2:43: +2:44
let mut _20: u128; // in scope 0 at $DIR/shifts.rs:+2:34: +2:44
let mut _21: bool; // in scope 0 at $DIR/shifts.rs:+2:34: +2:44
let mut _22: [u128; 3]; // in scope 0 at $DIR/shifts.rs:+3:9: +3:39
let mut _23: u128; // in scope 0 at $DIR/shifts.rs:+3:10: +3:18
let mut _24: u128; // in scope 0 at $DIR/shifts.rs:+3:10: +3:13
let mut _25: i8; // in scope 0 at $DIR/shifts.rs:+3:17: +3:18
let mut _26: u8; // in scope 0 at $DIR/shifts.rs:+3:10: +3:18
let mut _27: bool; // in scope 0 at $DIR/shifts.rs:+3:10: +3:18
let mut _28: u128; // in scope 0 at $DIR/shifts.rs:+3:20: +3:28
let mut _29: u128; // in scope 0 at $DIR/shifts.rs:+3:20: +3:23
let mut _30: i32; // in scope 0 at $DIR/shifts.rs:+3:27: +3:28
let mut _31: u32; // in scope 0 at $DIR/shifts.rs:+3:20: +3:28
let mut _32: bool; // in scope 0 at $DIR/shifts.rs:+3:20: +3:28
let mut _33: u128; // in scope 0 at $DIR/shifts.rs:+3:30: +3:38
let mut _34: u128; // in scope 0 at $DIR/shifts.rs:+3:30: +3:33
let mut _35: i128; // in scope 0 at $DIR/shifts.rs:+3:37: +3:38
let mut _36: u128; // in scope 0 at $DIR/shifts.rs:+3:30: +3:38
let mut _37: bool; // in scope 0 at $DIR/shifts.rs:+3:30: +3:38

bb0: {
StorageLive(_6); // scope 0 at $DIR/shifts.rs:+2:9: +2:45
StorageLive(_7); // scope 0 at $DIR/shifts.rs:+2:10: +2:20
StorageLive(_8); // scope 0 at $DIR/shifts.rs:+2:10: +2:15
_8 = _1; // scope 0 at $DIR/shifts.rs:+2:10: +2:15
StorageLive(_9); // scope 0 at $DIR/shifts.rs:+2:19: +2:20
_9 = _3; // scope 0 at $DIR/shifts.rs:+2:19: +2:20
_10 = _9 as u8 (IntToInt); // scope 0 at $DIR/shifts.rs:+2:10: +2:20
_11 = Lt(move _10, const 8_u8); // scope 0 at $DIR/shifts.rs:+2:10: +2:20
assert(move _11, "attempt to shift right by `{}`, which would overflow", _9) -> [success: bb1, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+2:10: +2:20
}

bb1: {
_7 = Shr(move _8, move _9); // scope 0 at $DIR/shifts.rs:+2:10: +2:20
StorageDead(_9); // scope 0 at $DIR/shifts.rs:+2:19: +2:20
StorageDead(_8); // scope 0 at $DIR/shifts.rs:+2:19: +2:20
StorageLive(_12); // scope 0 at $DIR/shifts.rs:+2:22: +2:32
StorageLive(_13); // scope 0 at $DIR/shifts.rs:+2:22: +2:27
_13 = _1; // scope 0 at $DIR/shifts.rs:+2:22: +2:27
StorageLive(_14); // scope 0 at $DIR/shifts.rs:+2:31: +2:32
_14 = _4; // scope 0 at $DIR/shifts.rs:+2:31: +2:32
_15 = _14 as u32 (IntToInt); // scope 0 at $DIR/shifts.rs:+2:22: +2:32
_16 = Lt(move _15, const 8_u32); // scope 0 at $DIR/shifts.rs:+2:22: +2:32
assert(move _16, "attempt to shift right by `{}`, which would overflow", _14) -> [success: bb2, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+2:22: +2:32
}

bb2: {
_12 = Shr(move _13, move _14); // scope 0 at $DIR/shifts.rs:+2:22: +2:32
StorageDead(_14); // scope 0 at $DIR/shifts.rs:+2:31: +2:32
StorageDead(_13); // scope 0 at $DIR/shifts.rs:+2:31: +2:32
StorageLive(_17); // scope 0 at $DIR/shifts.rs:+2:34: +2:44
StorageLive(_18); // scope 0 at $DIR/shifts.rs:+2:34: +2:39
_18 = _1; // scope 0 at $DIR/shifts.rs:+2:34: +2:39
StorageLive(_19); // scope 0 at $DIR/shifts.rs:+2:43: +2:44
_19 = _5; // scope 0 at $DIR/shifts.rs:+2:43: +2:44
_20 = _19 as u128 (IntToInt); // scope 0 at $DIR/shifts.rs:+2:34: +2:44
_21 = Lt(move _20, const 8_u128); // scope 0 at $DIR/shifts.rs:+2:34: +2:44
assert(move _21, "attempt to shift right by `{}`, which would overflow", _19) -> [success: bb3, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+2:34: +2:44
}

bb3: {
_17 = Shr(move _18, move _19); // scope 0 at $DIR/shifts.rs:+2:34: +2:44
StorageDead(_19); // scope 0 at $DIR/shifts.rs:+2:43: +2:44
StorageDead(_18); // scope 0 at $DIR/shifts.rs:+2:43: +2:44
_6 = [move _7, move _12, move _17]; // scope 0 at $DIR/shifts.rs:+2:9: +2:45
StorageDead(_17); // scope 0 at $DIR/shifts.rs:+2:44: +2:45
StorageDead(_12); // scope 0 at $DIR/shifts.rs:+2:44: +2:45
StorageDead(_7); // scope 0 at $DIR/shifts.rs:+2:44: +2:45
StorageLive(_22); // scope 0 at $DIR/shifts.rs:+3:9: +3:39
StorageLive(_23); // scope 0 at $DIR/shifts.rs:+3:10: +3:18
StorageLive(_24); // scope 0 at $DIR/shifts.rs:+3:10: +3:13
_24 = _2; // scope 0 at $DIR/shifts.rs:+3:10: +3:13
StorageLive(_25); // scope 0 at $DIR/shifts.rs:+3:17: +3:18
_25 = _3; // scope 0 at $DIR/shifts.rs:+3:17: +3:18
_26 = _25 as u8 (IntToInt); // scope 0 at $DIR/shifts.rs:+3:10: +3:18
_27 = Lt(move _26, const 128_u8); // scope 0 at $DIR/shifts.rs:+3:10: +3:18
assert(move _27, "attempt to shift left by `{}`, which would overflow", _25) -> [success: bb4, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+3:10: +3:18
}

bb4: {
_23 = Shl(move _24, move _25); // scope 0 at $DIR/shifts.rs:+3:10: +3:18
StorageDead(_25); // scope 0 at $DIR/shifts.rs:+3:17: +3:18
StorageDead(_24); // scope 0 at $DIR/shifts.rs:+3:17: +3:18
StorageLive(_28); // scope 0 at $DIR/shifts.rs:+3:20: +3:28
StorageLive(_29); // scope 0 at $DIR/shifts.rs:+3:20: +3:23
_29 = _2; // scope 0 at $DIR/shifts.rs:+3:20: +3:23
StorageLive(_30); // scope 0 at $DIR/shifts.rs:+3:27: +3:28
_30 = _4; // scope 0 at $DIR/shifts.rs:+3:27: +3:28
_31 = _30 as u32 (IntToInt); // scope 0 at $DIR/shifts.rs:+3:20: +3:28
_32 = Lt(move _31, const 128_u32); // scope 0 at $DIR/shifts.rs:+3:20: +3:28
assert(move _32, "attempt to shift left by `{}`, which would overflow", _30) -> [success: bb5, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+3:20: +3:28
}

bb5: {
_28 = Shl(move _29, move _30); // scope 0 at $DIR/shifts.rs:+3:20: +3:28
StorageDead(_30); // scope 0 at $DIR/shifts.rs:+3:27: +3:28
StorageDead(_29); // scope 0 at $DIR/shifts.rs:+3:27: +3:28
StorageLive(_33); // scope 0 at $DIR/shifts.rs:+3:30: +3:38
StorageLive(_34); // scope 0 at $DIR/shifts.rs:+3:30: +3:33
_34 = _2; // scope 0 at $DIR/shifts.rs:+3:30: +3:33
StorageLive(_35); // scope 0 at $DIR/shifts.rs:+3:37: +3:38
_35 = _5; // scope 0 at $DIR/shifts.rs:+3:37: +3:38
_36 = _35 as u128 (IntToInt); // scope 0 at $DIR/shifts.rs:+3:30: +3:38
_37 = Lt(move _36, const 128_u128); // scope 0 at $DIR/shifts.rs:+3:30: +3:38
assert(move _37, "attempt to shift left by `{}`, which would overflow", _35) -> [success: bb6, unwind: bb7]; // scope 0 at $DIR/shifts.rs:+3:30: +3:38
}

bb6: {
_33 = Shl(move _34, move _35); // scope 0 at $DIR/shifts.rs:+3:30: +3:38
StorageDead(_35); // scope 0 at $DIR/shifts.rs:+3:37: +3:38
StorageDead(_34); // scope 0 at $DIR/shifts.rs:+3:37: +3:38
_22 = [move _23, move _28, move _33]; // scope 0 at $DIR/shifts.rs:+3:9: +3:39
StorageDead(_33); // scope 0 at $DIR/shifts.rs:+3:38: +3:39
StorageDead(_28); // scope 0 at $DIR/shifts.rs:+3:38: +3:39
StorageDead(_23); // scope 0 at $DIR/shifts.rs:+3:38: +3:39
_0 = (move _6, move _22); // scope 0 at $DIR/shifts.rs:+1:5: +4:6
StorageDead(_22); // scope 0 at $DIR/shifts.rs:+4:5: +4:6
StorageDead(_6); // scope 0 at $DIR/shifts.rs:+4:5: +4:6
return; // scope 0 at $DIR/shifts.rs:+5:2: +5:2
}

bb7 (cleanup): {
resume; // scope 0 at $DIR/shifts.rs:+0:1: +5:2
}
}

0 comments on commit 2603ef0

Please sign in to comment.