Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added MIR constant propagation of Scalars into function call arguments #71697

Merged
merged 1 commit into from
May 4, 2020
Merged

Added MIR constant propagation of Scalars into function call arguments #71697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 51 additions & 5 deletions src/librustc_mir/transform/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,7 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
| NonMutatingUse(NonMutatingUseContext::Inspect)
| NonMutatingUse(NonMutatingUseContext::Projection)
| NonUse(_) => {}
// FIXME(felix91gr): explain the reasoning behind this
MutatingUse(MutatingUseContext::Projection) => {
if self.local_kinds[local] != LocalKind::Temp {
self.can_const_prop[local] = ConstPropMode::NoPropagation;
Expand Down Expand Up @@ -969,13 +970,58 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
| TerminatorKind::GeneratorDrop
| TerminatorKind::FalseEdges { .. }
| TerminatorKind::FalseUnwind { .. } => {}
//FIXME(wesleywiser) Call does have Operands that could be const-propagated
TerminatorKind::Call { .. } => {}
// Every argument in our function calls can be const propagated.
TerminatorKind::Call { ref mut args, .. } => {
let mir_opt_level = self.tcx.sess.opts.debugging_opts.mir_opt_level;
// Constant Propagation into function call arguments is gated
// under mir-opt-level 2, because LLVM codegen gives performance
// regressions with it.
if mir_opt_level >= 2 {
felix91gr marked this conversation as resolved.
Show resolved Hide resolved
for opr in args {
/*
The following code would appear to be incomplete, because
the function `Operand::place()` returns `None` if the
`Operand` is of the variant `Operand::Constant`. In this
context however, that variant will never appear. This is why:
Copy link
Contributor

Choose a reason for hiding this comment

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

The 42 is a const in the MIR's function call arguments in

fn main() {
    foo(42);
}

fn foo(i: i32) {}

But that's already "propagated" so it's fine and we don't need to care about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It took me a while to realize what you meant. But indeed! Those are already "propagated": https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9bfca57ccf6c252f7e601ed58beb7895

Should I add something to the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I think the comment should be adjusted. It's not that an Operand::Constant doesn't appear, it's that when it does, it's already constant and we don't need to propagate anything.


When constructing the MIR, all function call arguments are
copied into `Locals` of `LocalKind::Temp`. At least, all arguments
that are not unsized (Less than 0.1% are unsized. See #71170
to learn more about those).

This means that, conversely, all `Operands` found as function call
arguments are of the variant `Operand::Copy`. This allows us to
simplify our handling of `Operands` in this case.
*/
if let Some(l) = opr.place().and_then(|p| p.as_local()) {
if let Some(value) = self.get_const(l) {
felix91gr marked this conversation as resolved.
Show resolved Hide resolved
if self.should_const_prop(value) {
// FIXME(felix91gr): this code only handles `Scalar` cases.
// For now, we're not handling `ScalarPair` cases because
// doing so here would require a lot of code duplication.
// We should hopefully generalize `Operand` handling into a fn,
// and use it to do const-prop here and everywhere else
// where it makes sense.
if let interpret::Operand::Immediate(
interpret::Immediate::Scalar(
felix91gr marked this conversation as resolved.
Show resolved Hide resolved
interpret::ScalarMaybeUndef::Scalar(scalar),
),
) = *value
{
*opr = self.operand_from_scalar(
scalar,
value.layout.ty,
source_info.span,
);
}
}
}
}
}
}
}
}
// We remove all Locals which are restricted in propagation to their containing blocks.
// We wouldn't need to clone, but the borrow checker can't see that we're not aliasing
// the locals_of_current_block field, so we need to clone it first.
// let ecx = &mut self.ecx;
for local in self.locals_of_current_block.iter() {
Self::remove_const(&mut self.ecx, local);
}
Expand Down
13 changes: 10 additions & 3 deletions src/test/mir-opt/const_prop/scalar_literal_propagation/rustc.main.ConstProp.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,27 @@
StorageLive(_2); // scope 1 at $DIR/scalar_literal_propagation.rs:4:5: 4:15
StorageLive(_3); // scope 1 at $DIR/scalar_literal_propagation.rs:4:13: 4:14
- _3 = _1; // scope 1 at $DIR/scalar_literal_propagation.rs:4:13: 4:14
- _2 = const consume(move _3) -> bb1; // scope 1 at $DIR/scalar_literal_propagation.rs:4:5: 4:15
+ _3 = const 1u32; // scope 1 at $DIR/scalar_literal_propagation.rs:4:13: 4:14
+ // ty::Const
// ty::Const
+ // + ty: u32
+ // + val: Value(Scalar(0x00000001))
+ // mir::Constant
+ // + span: $DIR/scalar_literal_propagation.rs:4:13: 4:14
+ // + literal: Const { ty: u32, val: Value(Scalar(0x00000001)) }
_2 = const consume(move _3) -> bb1; // scope 1 at $DIR/scalar_literal_propagation.rs:4:5: 4:15
// ty::Const
+ _2 = const consume(const 1u32) -> bb1; // scope 1 at $DIR/scalar_literal_propagation.rs:4:5: 4:15
+ // ty::Const
// + ty: fn(u32) {consume}
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/scalar_literal_propagation.rs:4:5: 4:12
// + literal: Const { ty: fn(u32) {consume}, val: Value(Scalar(<ZST>)) }
+ // ty::Const
+ // + ty: u32
+ // + val: Value(Scalar(0x00000001))
+ // mir::Constant
+ // + span: $DIR/scalar_literal_propagation.rs:4:5: 4:15
+ // + literal: Const { ty: u32, val: Value(Scalar(0x00000001)) }
}

bb1: {
Expand Down
22 changes: 20 additions & 2 deletions src/test/mir-opt/simplify-locals-removes-unused-consts/rustc.main.SimplifyLocals.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
- StorageDead(_2); // scope 0 at $DIR/simplify-locals-removes-unused-consts.rs:13:27: 13:28
- StorageDead(_1); // scope 0 at $DIR/simplify-locals-removes-unused-consts.rs:13:28: 13:29
- StorageLive(_4); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:5: 14:22
- StorageLive(_5); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:13: 14:21
- StorageLive(_6); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:14: 14:16
- _6 = const (); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:14: 14:16
- // ty::Const
Expand All @@ -66,6 +67,13 @@
- // mir::Constant
- // + span: $DIR/simplify-locals-removes-unused-consts.rs:14:18: 14:20
- // + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
- _5 = const ((), ()); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:13: 14:21
- // ty::Const
- // + ty: ((), ())
- // + val: Value(Scalar(<ZST>))
- // mir::Constant
- // + span: $DIR/simplify-locals-removes-unused-consts.rs:14:13: 14:21
- // + literal: Const { ty: ((), ()), val: Value(Scalar(<ZST>)) }
- StorageDead(_7); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:20: 14:21
- StorageDead(_6); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:20: 14:21
- _4 = const use_zst(const ((), ())) -> bb1; // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:5: 14:22
Expand All @@ -79,13 +87,15 @@
// + ty: ((), ())
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/simplify-locals-removes-unused-consts.rs:14:13: 14:21
// + span: $DIR/simplify-locals-removes-unused-consts.rs:14:5: 14:22
// + literal: Const { ty: ((), ()), val: Value(Scalar(<ZST>)) }
}

bb1: {
- StorageDead(_5); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:21: 14:22
- StorageDead(_4); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:14:22: 14:23
- StorageLive(_8); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:5: 16:35
- StorageLive(_9); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:34
- StorageLive(_10); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:30
- StorageLive(_11); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:28
- _11 = const Temp { x: 40u8 }; // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:28
Expand All @@ -105,6 +115,13 @@
- // mir::Constant
- // + span: $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:30
- // + literal: Const { ty: u8, val: Value(Scalar(0x28)) }
- _9 = const 42u8; // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:34
- // ty::Const
- // + ty: u8
- // + val: Value(Scalar(0x2a))
- // mir::Constant
- // + span: $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:34
- // + literal: Const { ty: u8, val: Value(Scalar(0x2a)) }
- StorageDead(_10); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:33: 16:34
- _8 = const use_u8(const 42u8) -> bb2; // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:5: 16:35
- // ty::Const
Expand All @@ -117,11 +134,12 @@
// + ty: u8
// + val: Value(Scalar(0x2a))
// mir::Constant
// + span: $DIR/simplify-locals-removes-unused-consts.rs:16:12: 16:34
// + span: $DIR/simplify-locals-removes-unused-consts.rs:16:5: 16:35
// + literal: Const { ty: u8, val: Value(Scalar(0x2a)) }
}

bb2: {
- StorageDead(_9); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:34: 16:35
- StorageDead(_11); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:35: 16:36
- StorageDead(_8); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:35: 16:36
+ StorageDead(_2); // scope 1 at $DIR/simplify-locals-removes-unused-consts.rs:16:35: 16:36
Expand Down