Skip to content

Commit

Permalink
const-prop: Restrict scalar pair propagation
Browse files Browse the repository at this point in the history
We now only propagate a scalar pair if the Rvalue is a tuple with two
scalars. This for example avoids propagating a (u8, u8) value when
Rvalue has type `((), u8, u8)` (see the regression test). While this is
a correct thing to do, implementation is tricky and will be done later.

Fixes #66971
Fixes #66339
Fixes #67019
  • Loading branch information
osa1 committed Dec 6, 2019
1 parent d0126e8 commit 2404a06
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 11 deletions.
48 changes: 37 additions & 11 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -636,19 +636,45 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ScalarMaybeUndef::Scalar(one),
ScalarMaybeUndef::Scalar(two)
) => {
// Found a value represented as a pair. For now only do cont-prop if type of
// Rvalue is also a pair with two scalars. The more general case is more
// complicated to implement so we'll do it later.
let ty = &value.layout.ty.kind;
// Only do it for tuples
if let ty::Tuple(substs) = ty {
*rval = Rvalue::Aggregate(
Box::new(AggregateKind::Tuple),
vec![
self.operand_from_scalar(
one, substs[0].expect_ty(), source_info.span
),
self.operand_from_scalar(
two, substs[1].expect_ty(), source_info.span
),
],
);
// Only do it if tuple is also a pair with two scalars
if substs.len() == 2 {
let opt_ty1_ty2 = self.use_ecx(source_info, |this| {
let ty1 = substs[0].expect_ty();
let ty2 = substs[1].expect_ty();
let ty_is_scalar = |ty| {
this.ecx
.layout_of(ty)
.ok()
.map(|ty| ty.details.abi.is_scalar())
== Some(true)
};
if ty_is_scalar(ty1) && ty_is_scalar(ty2) {
Ok(Some((ty1, ty2)))
} else {
Ok(None)
}
});

if let Some(Some((ty1, ty2))) = opt_ty1_ty2 {
*rval = Rvalue::Aggregate(
Box::new(AggregateKind::Tuple),
vec![
self.operand_from_scalar(
one, ty1, source_info.span
),
self.operand_from_scalar(
two, ty2, source_info.span
),
],
);
}
}
}
},
_ => { }
Expand Down
8 changes: 8 additions & 0 deletions src/librustc_target/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,14 @@ impl Abi {
_ => false,
}
}

/// Returns `true` is this is a scalar type
pub fn is_scalar(&self) -> bool {
match *self {
Abi::Scalar(_) => true,
_ => false,
}
}
}

rustc_index::newtype_index! {
Expand Down
38 changes: 38 additions & 0 deletions src/test/mir-opt/const_prop/issue-66971.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// compile-flags: -Z mir-opt-level=2

// Due to a bug in propagating scalar pairs the assertion below used to fail. In the expected
// outputs below, after ConstProp this is how _2 would look like with the bug:
//
// _2 = (const Scalar(0x00) : (), const 0u8);
//
// Which has the wrong type.

fn encode(this: ((), u8, u8)) {
assert!(this.2 == 0);
}

fn main() {
encode(((), 0, 0));
}

// END RUST SOURCE
// START rustc.main.ConstProp.before.mir
// bb0: {
// ...
// _3 = ();
// _2 = (move _3, const 0u8, const 0u8);
// ...
// _1 = const encode(move _2) -> bb1;
// ...
// }
// END rustc.main.ConstProp.before.mir
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _3 = const Scalar(<ZST>) : ();
// _2 = (move _3, const 0u8, const 0u8);
// ...
// _1 = const encode(move _2) -> bb1;
// ...
// }
// END rustc.main.ConstProp.after.mir
34 changes: 34 additions & 0 deletions src/test/mir-opt/const_prop/issue-67019.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// compile-flags: -Z mir-opt-level=2

// This used to ICE in const-prop

fn test(this: ((u8, u8),)) {
assert!((this.0).0 == 1);
}

fn main() {
test(((1, 2),));
}

// Important bit is parameter passing so we only check that below
// END RUST SOURCE
// START rustc.main.ConstProp.before.mir
// bb0: {
// ...
// _3 = (const 1u8, const 2u8);
// _2 = (move _3,);
// ...
// _1 = const test(move _2) -> bb1;
// ...
// }
// END rustc.main.ConstProp.before.mir
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _3 = (const 1u8, const 2u8);
// _2 = (move _3,);
// ...
// _1 = const test(move _2) -> bb1;
// ...
// }
// END rustc.main.ConstProp.after.mir
13 changes: 13 additions & 0 deletions src/test/ui/mir/issue66339.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// compile-flags: -Z mir-opt-level=2
// build-pass

// This used to ICE in const-prop

fn foo() {
let bar = |_| { };
let _ = bar("a");
}

fn main() {
foo();
}

0 comments on commit 2404a06

Please sign in to comment.