From 2404a067eedd83ab69bb0e07fdc8145825741722 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20Sinan=20A=C4=9Facan?= Date: Thu, 5 Dec 2019 10:40:24 +0300 Subject: [PATCH] const-prop: Restrict scalar pair propagation 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 --- src/librustc_mir/transform/const_prop.rs | 48 +++++++++++++++++----- src/librustc_target/abi/mod.rs | 8 ++++ src/test/mir-opt/const_prop/issue-66971.rs | 38 +++++++++++++++++ src/test/mir-opt/const_prop/issue-67019.rs | 34 +++++++++++++++ src/test/ui/mir/issue66339.rs | 13 ++++++ 5 files changed, 130 insertions(+), 11 deletions(-) create mode 100644 src/test/mir-opt/const_prop/issue-66971.rs create mode 100644 src/test/mir-opt/const_prop/issue-67019.rs create mode 100644 src/test/ui/mir/issue66339.rs diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 95de635d634e4..a147ad3bb1e22 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -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 + ), + ], + ); + } + } } }, _ => { } diff --git a/src/librustc_target/abi/mod.rs b/src/librustc_target/abi/mod.rs index ac781819cc35e..b0bc052929768 100644 --- a/src/librustc_target/abi/mod.rs +++ b/src/librustc_target/abi/mod.rs @@ -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! { diff --git a/src/test/mir-opt/const_prop/issue-66971.rs b/src/test/mir-opt/const_prop/issue-66971.rs new file mode 100644 index 0000000000000..30c75303b3e53 --- /dev/null +++ b/src/test/mir-opt/const_prop/issue-66971.rs @@ -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() : (); +// _2 = (move _3, const 0u8, const 0u8); +// ... +// _1 = const encode(move _2) -> bb1; +// ... +// } +// END rustc.main.ConstProp.after.mir diff --git a/src/test/mir-opt/const_prop/issue-67019.rs b/src/test/mir-opt/const_prop/issue-67019.rs new file mode 100644 index 0000000000000..c6d753a1209cd --- /dev/null +++ b/src/test/mir-opt/const_prop/issue-67019.rs @@ -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 diff --git a/src/test/ui/mir/issue66339.rs b/src/test/ui/mir/issue66339.rs new file mode 100644 index 0000000000000..98e178c055146 --- /dev/null +++ b/src/test/ui/mir/issue66339.rs @@ -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(); +}