Skip to content

Commit e34caaf

Browse files
cjgillotcjgillot
committed
Remove overflow checks from ConstProp.
1 parent 4bd2ebc commit e34caaf

File tree

8 files changed

+88
-116
lines changed

8 files changed

+88
-116
lines changed

‎compiler/rustc_mir_transform/src/const_prop.rs‎

Lines changed: 15 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ use rustc_middle::mir::visit::{
1515
};
1616
use rustc_middle::mir::{
1717
BasicBlock, BinOp, Body, Constant, ConstantKind, Local, LocalDecl, LocalKind, Location,
18-
Operand, Place, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind, UnOp,
18+
Operand, Place, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind,
1919
RETURN_PLACE,
2020
};
2121
use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout};
@@ -503,55 +503,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
503503
}
504504
}
505505

506-
fn check_unary_op(&mut self, op: UnOp, arg: &Operand<'tcx>) -> Option<()> {
507-
if self.use_ecx(|this| {
508-
let val = this.ecx.read_immediate(&this.ecx.eval_operand(arg, None)?)?;
509-
let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, &val)?;
510-
Ok(overflow)
511-
})? {
512-
// `AssertKind` only has an `OverflowNeg` variant, so make sure that is
513-
// appropriate to use.
514-
assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow");
515-
return None;
516-
}
517-
518-
Some(())
519-
}
520-
521-
fn check_binary_op(
522-
&mut self,
523-
op: BinOp,
524-
left: &Operand<'tcx>,
525-
right: &Operand<'tcx>,
526-
) -> Option<()> {
527-
let r = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?));
528-
let l = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?));
529-
// Check for exceeding shifts *even if* we cannot evaluate the LHS.
530-
if matches!(op, BinOp::Shr | BinOp::Shl) {
531-
let r = r.clone()?;
532-
// We need the type of the LHS. We cannot use `place_layout` as that is the type
533-
// of the result, which for checked binops is not the same!
534-
let left_ty = left.ty(self.local_decls, self.tcx);
535-
let left_size = self.ecx.layout_of(left_ty).ok()?.size;
536-
let right_size = r.layout.size;
537-
let r_bits = r.to_scalar().to_bits(right_size).ok();
538-
if r_bits.map_or(false, |b| b >= left_size.bits() as u128) {
539-
return None;
540-
}
541-
}
542-
543-
if let (Some(l), Some(r)) = (&l, &r) {
544-
// The remaining operators are handled through `overflowing_binary_op`.
545-
if self.use_ecx(|this| {
546-
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
547-
Ok(overflow)
548-
})? {
549-
return None;
550-
}
551-
}
552-
Some(())
553-
}
554-
555506
fn propagate_operand(&mut self, operand: &mut Operand<'tcx>) {
556507
match *operand {
557508
Operand::Copy(l) | Operand::Move(l) => {
@@ -587,28 +538,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
587538
// 2. Working around bugs in other parts of the compiler
588539
// - In this case, we'll return `None` from this function to stop evaluation.
589540
match rvalue {
590-
// Additional checking: give lints to the user if an overflow would occur.
591-
// We do this here and not in the `Assert` terminator as that terminator is
592-
// only sometimes emitted (overflow checks can be disabled), but we want to always
593-
// lint.
594-
Rvalue::UnaryOp(op, arg) => {
595-
trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg);
596-
self.check_unary_op(*op, arg)?;
597-
}
598-
Rvalue::BinaryOp(op, box (left, right)) => {
599-
trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right);
600-
self.check_binary_op(*op, left, right)?;
601-
}
602-
Rvalue::CheckedBinaryOp(op, box (left, right)) => {
603-
trace!(
604-
"checking CheckedBinaryOp(op = {:?}, left = {:?}, right = {:?})",
605-
op,
606-
left,
607-
right
608-
);
609-
self.check_binary_op(*op, left, right)?;
610-
}
611-
612541
// Do not try creating references (#67862)
613542
Rvalue::AddressOf(_, place) | Rvalue::Ref(_, _, place) => {
614543
trace!("skipping AddressOf | Ref for {:?}", place);
@@ -638,7 +567,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
638567
| Rvalue::Cast(..)
639568
| Rvalue::ShallowInitBox(..)
640569
| Rvalue::Discriminant(..)
641-
| Rvalue::NullaryOp(..) => {}
570+
| Rvalue::NullaryOp(..)
571+
| Rvalue::UnaryOp(..)
572+
| Rvalue::BinaryOp(..)
573+
| Rvalue::CheckedBinaryOp(..) => {}
642574
}
643575

644576
// FIXME we need to revisit this for #67176
@@ -1079,31 +1011,18 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
10791011
// Do NOT early return in this function, it does some crucial fixup of the state at the end!
10801012
match &mut terminator.kind {
10811013
TerminatorKind::Assert { expected, ref mut cond, .. } => {
1082-
if let Some(ref value) = self.eval_operand(&cond) {
1083-
trace!("assertion on {:?} should be {:?}", value, expected);
1084-
let expected = Scalar::from_bool(*expected);
1014+
if let Some(ref value) = self.eval_operand(&cond)
10851015
// FIXME should be used use_ecx rather than a local match... but we have
10861016
// quite a few of these read_scalar/read_immediate that need fixing.
1087-
if let Ok(value_const) = self.ecx.read_scalar(&value) {
1088-
if expected != value_const {
1089-
// Poison all places this operand references so that further code
1090-
// doesn't use the invalid value
1091-
match cond {
1092-
Operand::Move(ref place) | Operand::Copy(ref place) => {
1093-
Self::remove_const(&mut self.ecx, place.local);
1094-
}
1095-
Operand::Constant(_) => {}
1096-
}
1097-
} else {
1098-
if self.should_const_prop(value) {
1099-
*cond = self.operand_from_scalar(
1100-
value_const,
1101-
self.tcx.types.bool,
1102-
source_info.span,
1103-
);
1104-
}
1105-
}
1106-
}
1017+
&& let Ok(value_const) = self.ecx.read_scalar(&value)
1018+
&& self.should_const_prop(value)
1019+
{
1020+
trace!("assertion on {:?} should be {:?}", value, expected);
1021+
*cond = self.operand_from_scalar(
1022+
value_const,
1023+
self.tcx.types.bool,
1024+
source_info.span,
1025+
);
11071026
}
11081027
}
11091028
TerminatorKind::SwitchInt { ref mut discr, .. } => {

‎compiler/rustc_mir_transform/src/dataflow_const_prop.rs‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -180,12 +180,7 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> {
180180
let overflow = match overflow {
181181
FlatSet::Top => FlatSet::Top,
182182
FlatSet::Elem(overflow) => {
183-
if overflow {
184-
// Overflow cannot be reliably propagated. See: https://github.com/rust-lang/rust/pull/101168#issuecomment-1288091446
185-
FlatSet::Top
186-
} else {
187-
self.wrap_scalar(Scalar::from_bool(false), self.tcx.types.bool)
188-
}
183+
self.wrap_scalar(Scalar::from_bool(overflow), self.tcx.types.bool)
189184
}
190185
FlatSet::Bottom => FlatSet::Bottom,
191186
};

‎tests/mir-opt/const_prop/bad_op_div_by_zero.main.ConstProp.diff‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,10 @@
2424
StorageLive(_3); // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:18: +2:19
2525
- _3 = _1; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:18: +2:19
2626
- _4 = Eq(_3, const 0_i32); // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
27+
- assert(!move _4, "attempt to divide `{}` by zero", const 1_i32) -> bb1; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
2728
+ _3 = const 0_i32; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:18: +2:19
2829
+ _4 = const true; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
29-
assert(!move _4, "attempt to divide `{}` by zero", const 1_i32) -> bb1; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
30+
+ assert(!const true, "attempt to divide `{}` by zero", const 1_i32) -> bb1; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
3031
}
3132

3233
bb1: {

‎tests/mir-opt/const_prop/inherit_overflow.main.ConstProp.diff‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
- // MIR for `main` before ConstProp
2+
+ // MIR for `main` after ConstProp
3+
4+
fn main() -> () {
5+
let mut _0: (); // return place in scope 0 at $DIR/inherit_overflow.rs:+0:11: +0:11
6+
let mut _1: u8; // in scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
7+
let mut _2: u8; // in scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
8+
let mut _3: u8; // in scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
9+
scope 1 {
10+
}
11+
scope 2 (inlined <u8 as Add>::add) { // at $DIR/inherit_overflow.rs:8:13: 8:47
12+
debug self => _2; // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
13+
debug other => _3; // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
14+
let mut _4: (u8, bool); // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
15+
}
16+
17+
bb0: {
18+
StorageLive(_1); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
19+
StorageLive(_2); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
20+
_2 = const u8::MAX; // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
21+
StorageLive(_3); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
22+
_3 = const 1_u8; // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
23+
- _4 = CheckedAdd(_2, _3); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
24+
- assert(!move (_4.1: bool), "attempt to compute `{} + {}`, which would overflow", _2, _3) -> bb1; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
25+
+ _4 = const (0_u8, true); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
26+
+ assert(!const true, "attempt to compute `{} + {}`, which would overflow", _2, _3) -> bb1; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
27+
}
28+
29+
bb1: {
30+
- _1 = move (_4.0: u8); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
31+
+ _1 = const 0_u8; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
32+
StorageDead(_3); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
33+
StorageDead(_2); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
34+
StorageDead(_1); // scope 0 at $DIR/inherit_overflow.rs:+3:47: +3:48
35+
_0 = const (); // scope 0 at $DIR/inherit_overflow.rs:+0:11: +4:2
36+
return; // scope 0 at $DIR/inherit_overflow.rs:+4:2: +4:2
37+
}
38+
}
39+

‎tests/mir-opt/const_prop/inherit_overflow.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// unit-test: ConstProp
2+
// compile-flags: -Zmir-enable-passes=+Inline
3+
4+
// EMIT_MIR inherit_overflow.main.ConstProp.diff
5+
fn main() {
6+
// After inlining, this will contain a `CheckedBinaryOp`.
7+
// Propagating the overflow is ok as codegen will just skip emitting the panic.
8+
let _ = <u8 as std::ops::Add>::add(255, 1);
9+
}

‎tests/mir-opt/dataflow-const-prop/checked.main.DataflowConstProp.diff‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161
- assert(!move (_10.1: bool), "attempt to compute `{} + {}`, which would overflow", move _9, const 1_i32) -> bb2; // scope 4 at $DIR/checked.rs:+6:13: +6:18
6262
+ _9 = const i32::MAX; // scope 4 at $DIR/checked.rs:+6:13: +6:14
6363
+ _10 = CheckedAdd(const i32::MAX, const 1_i32); // scope 4 at $DIR/checked.rs:+6:13: +6:18
64-
+ assert(!move (_10.1: bool), "attempt to compute `{} + {}`, which would overflow", const i32::MAX, const 1_i32) -> bb2; // scope 4 at $DIR/checked.rs:+6:13: +6:18
64+
+ assert(!const true, "attempt to compute `{} + {}`, which would overflow", const i32::MAX, const 1_i32) -> bb2; // scope 4 at $DIR/checked.rs:+6:13: +6:18
6565
}
6666

6767
bb2: {

‎tests/mir-opt/dataflow-const-prop/inherit_overflow.main.DataflowConstProp.diff‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,26 +5,34 @@
55
let mut _0: (); // return place in scope 0 at $DIR/inherit_overflow.rs:+0:11: +0:11
66
let mut _1: u8; // in scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
77
let mut _2: u8; // in scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
8+
let mut _3: u8; // in scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
89
scope 1 {
910
}
10-
scope 2 (inlined <u8 as Add>::add) { // at $DIR/inherit_overflow.rs:7:13: 7:47
11-
debug self => _1; // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
12-
debug other => _2; // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
13-
let mut _3: (u8, bool); // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
11+
scope 2 (inlined <u8 as Add>::add) { // at $DIR/inherit_overflow.rs:8:13: 8:47
12+
debug self => _2; // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
13+
debug other => _3; // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
14+
let mut _4: (u8, bool); // in scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
1415
}
1516

1617
bb0: {
1718
StorageLive(_1); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
18-
_1 = const u8::MAX; // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
1919
StorageLive(_2); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
20-
_2 = const 1_u8; // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
21-
_3 = CheckedAdd(const u8::MAX, const 1_u8); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
22-
assert(!move (_3.1: bool), "attempt to compute `{} + {}`, which would overflow", const u8::MAX, const 1_u8) -> bb1; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
20+
_2 = const u8::MAX; // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
21+
StorageLive(_3); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
22+
_3 = const 1_u8; // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
23+
- _4 = CheckedAdd(_2, _3); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
24+
- assert(!move (_4.1: bool), "attempt to compute `{} + {}`, which would overflow", _2, _3) -> bb1; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
25+
+ _4 = CheckedAdd(const u8::MAX, const 1_u8); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
26+
+ assert(!const true, "attempt to compute `{} + {}`, which would overflow", const u8::MAX, const 1_u8) -> bb1; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
2327
}
2428

2529
bb1: {
30+
- _1 = move (_4.0: u8); // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
31+
+ _1 = const 0_u8; // scope 2 at $SRC_DIR/core/src/ops/arith.rs:LL:COL
32+
StorageDead(_3); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
2633
StorageDead(_2); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
27-
StorageDead(_1); // scope 0 at $DIR/inherit_overflow.rs:+3:13: +3:47
34+
StorageDead(_1); // scope 0 at $DIR/inherit_overflow.rs:+3:47: +3:48
35+
_0 = const (); // scope 0 at $DIR/inherit_overflow.rs:+0:11: +4:2
2836
return; // scope 0 at $DIR/inherit_overflow.rs:+4:2: +4:2
2937
}
3038
}

‎tests/mir-opt/dataflow-const-prop/inherit_overflow.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
// compile-flags: -Zunsound-mir-opts
1+
// unit-test: DataflowConstProp
2+
// compile-flags: -Zmir-enable-passes=+Inline
23

34
// EMIT_MIR inherit_overflow.main.DataflowConstProp.diff
45
fn main() {
5-
// After inlining, this will contain a `CheckedBinaryOp`. The overflow
6-
// must be ignored by the constant propagation to avoid triggering a panic.
6+
// After inlining, this will contain a `CheckedBinaryOp`.
7+
// Propagating the overflow is ok as codegen will just skip emitting the panic.
78
let _ = <u8 as std::ops::Add>::add(255, 1);
89
}

0 commit comments

Comments
 (0)