Skip to content

Fixed const propagation not working in a particular scenario #71298

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

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

Already on GitHub? # to your account

Closed
wants to merge 7 commits into from
Closed

Fixed const propagation not working in a particular scenario #71298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
837c29b
Added regression test for literal propagation and for scalar pair pro…
felix91gr Apr 23, 2020
faa3fbb
Blocked `MutatingUseContext::Projection` for all locals of kind `Loca…
felix91gr Apr 23, 2020
e541d81
Allow Locals to be propagated into and from, but restricted to their …
felix91gr Apr 23, 2020
f64a0e4
Allow constant propagation into function call arguments
felix91gr Apr 23, 2020
45f1775
Polished documentation, removed not-so-useful-anymore traces, and add…
felix91gr Apr 23, 2020
8a8c8c9
Remove unnecessary `.clone()`, by making `remove_const` not alias wit…
felix91gr Apr 26, 2020
6e586b8
Simplify fn call const propagation (by wesleywiser)
felix91gr Apr 26, 2020
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
123 changes: 100 additions & 23 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 @@ -334,6 +334,8 @@ struct ConstPropagator<'mir, 'tcx> {
// Because we have `MutVisitor` we can't obtain the `SourceInfo` from a `Location`. So we store
// the last known `SourceInfo` here and just keep revisiting it.
source_info: Option<SourceInfo>,
// Locals we need to forget at the end of the current block
locals_of_current_block: BitSet<Local>,
}

impl<'mir, 'tcx> LayoutOf for ConstPropagator<'mir, 'tcx> {
Expand Down Expand Up @@ -404,6 +406,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
local_decls: body.local_decls.clone(),
ret: ret.map(Into::into),
source_info: None,
locals_of_current_block: BitSet::new_empty(body.local_decls.len()),
}
}

Expand All @@ -420,8 +423,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
self.ecx.access_local(self.ecx.frame(), local, None).ok()
}

fn remove_const(&mut self, local: Local) {
self.ecx.frame_mut().locals[local] =
/// Remove `local` from the pool of `Locals`. Allows writing to them,
/// but not reading from them anymore.
fn remove_const(ecx: &mut InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, local: Local) {
ecx.frame_mut().locals[local] =
LocalState { value: LocalValue::Uninitialized, layout: Cell::new(None) };
}

Expand Down Expand Up @@ -452,6 +457,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}
}

/// Returns the value, if any, of evaluating `c`.
fn eval_constant(&mut self, c: &Constant<'tcx>, source_info: SourceInfo) -> Option<OpTy<'tcx>> {
// FIXME we need to revisit this for #67176
if c.needs_subst() {
Expand Down Expand Up @@ -492,11 +498,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}
}

/// Returns the value, if any, of evaluating `place`.
fn eval_place(&mut self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
trace!("eval_place(place={:?})", place);
self.use_ecx(|this| this.ecx.eval_place_to_op(place, None))
}

/// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant`
/// or `eval_place`, depending on the variant of `Operand` used.
fn eval_operand(&mut self, op: &Operand<'tcx>, source_info: SourceInfo) -> Option<OpTy<'tcx>> {
match *op {
Operand::Constant(ref c) => self.eval_constant(c, source_info),
Expand Down Expand Up @@ -655,6 +664,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
})
}

/// Creates a new `Operand::Constant` from a `Scalar` value
fn operand_from_scalar(&self, scalar: Scalar, ty: Ty<'tcx>, span: Span) -> Operand<'tcx> {
Operand::Constant(Box::new(Constant {
span,
Expand Down Expand Up @@ -700,6 +710,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// 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.
// FIXME: implement the general case stated above ^.
let ty = &value.layout.ty.kind;
// Only do it for tuples
if let ty::Tuple(substs) = ty {
Expand Down Expand Up @@ -736,6 +747,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}
}

/// Returns `true` if and only if this `op` should be const-propagated into.
fn should_const_prop(&mut self, op: OpTy<'tcx>) -> bool {
let mir_opt_level = self.tcx.sess.opts.debugging_opts.mir_opt_level;

Expand Down Expand Up @@ -767,6 +779,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
enum ConstPropMode {
/// The `Local` can be propagated into and reads of this `Local` can also be propagated.
FullConstProp,
/// The `Local` can only be propagated into and from its own block.
OnlyInsideOwnBlock,
/// The `Local` can be propagated into but reads cannot be propagated.
OnlyPropagateInto,
/// No propagation is allowed at all.
Expand All @@ -775,28 +789,41 @@ enum ConstPropMode {

struct CanConstProp {
can_const_prop: IndexVec<Local, ConstPropMode>,
// false at the beginning, once set, there are not allowed to be any more assignments
// False at the beginning. Once set, no more assignments are allowed to that local.
found_assignment: BitSet<Local>,
// Cache of locals' information
local_kinds: IndexVec<Local, LocalKind>,
}

impl CanConstProp {
/// returns true if `local` can be propagated
/// Returns true if `local` can be propagated
fn check(body: &Body<'_>) -> IndexVec<Local, ConstPropMode> {
let mut cpv = CanConstProp {
can_const_prop: IndexVec::from_elem(ConstPropMode::FullConstProp, &body.local_decls),
found_assignment: BitSet::new_empty(body.local_decls.len()),
local_kinds: IndexVec::from_fn_n(
|local| body.local_kind(local),
body.local_decls.len(),
),
};
for (local, val) in cpv.can_const_prop.iter_enumerated_mut() {
// cannot use args at all
// cannot use locals because if x < y { y - x } else { x - y } would
// Cannot use args at all
// Cannot use locals because if x < y { y - x } else { x - y } would
// lint for x != y
// FIXME(oli-obk): lint variables until they are used in a condition
// FIXME(oli-obk): lint if return value is constant
let local_kind = body.local_kind(local);

if local_kind == LocalKind::Arg || local_kind == LocalKind::Var {
if cpv.local_kinds[local] == LocalKind::Arg {
*val = ConstPropMode::OnlyPropagateInto;
trace!("local {:?} can't be const propagated because it's not a temporary", local);
trace!(
"local {:?} can't be const propagated because it's a function argument",
local
);
} else if cpv.local_kinds[local] == LocalKind::Var {
*val = ConstPropMode::OnlyInsideOwnBlock;
trace!(
"local {:?} will only be propagated inside its block, because it's a user variable",
local
);
}
}
cpv.visit_body(&body);
Expand All @@ -822,8 +849,12 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
| NonMutatingUse(NonMutatingUseContext::Move)
| NonMutatingUse(NonMutatingUseContext::Inspect)
| NonMutatingUse(NonMutatingUseContext::Projection)
| MutatingUse(MutatingUseContext::Projection)
| NonUse(_) => {}
MutatingUse(MutatingUseContext::Projection) => {
if self.local_kinds[local] != LocalKind::Temp {
self.can_const_prop[local] = ConstPropMode::NoPropagation;
}
}
_ => {
trace!("local {:?} can't be propagaged because it's used: {:?}", local, context);
self.can_const_prop[local] = ConstPropMode::NoPropagation;
Expand Down Expand Up @@ -854,25 +885,35 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
if let Some(local) = place.as_local() {
let can_const_prop = self.can_const_prop[local];
if let Some(()) = self.const_prop(rval, place_layout, source_info, place) {
if can_const_prop == ConstPropMode::FullConstProp
|| can_const_prop == ConstPropMode::OnlyPropagateInto
{
if can_const_prop != ConstPropMode::NoPropagation {
// This will return None for Locals that are from other blocks,
// so it should be okay to propagate from here on down.
if let Some(value) = self.get_const(local) {
if self.should_const_prop(value) {
trace!("replacing {:?} with {:?}", rval, value);
self.replace_with_const(rval, value, statement.source_info);

if can_const_prop == ConstPropMode::FullConstProp {
if can_const_prop == ConstPropMode::FullConstProp
|| can_const_prop == ConstPropMode::OnlyInsideOwnBlock
{
trace!("propagated into {:?}", local);
}
}
if can_const_prop == ConstPropMode::OnlyInsideOwnBlock {
trace!(
"found local restricted to its block. Will remove it from const-prop after block is finished. Local: {:?}",
local
);
self.locals_of_current_block.insert(local);
}
}
}
}
if self.can_const_prop[local] != ConstPropMode::FullConstProp {
if self.can_const_prop[local] == ConstPropMode::OnlyPropagateInto
|| self.can_const_prop[local] == ConstPropMode::NoPropagation
{
trace!("can't propagate into {:?}", local);
if local != RETURN_PLACE {
self.remove_const(local);
Self::remove_const(&mut self.ecx, local);
}
}
}
Expand Down Expand Up @@ -907,11 +948,11 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
let expected = ScalarMaybeUndef::from(Scalar::from_bool(*expected));
let value_const = self.ecx.read_scalar(value).unwrap();
if expected != value_const {
// poison all places this operand references so that further code
// Poison all places this operand references so that further code
// doesn't use the invalid value
match cond {
Operand::Move(ref place) | Operand::Copy(ref place) => {
self.remove_const(place.local);
Self::remove_const(&mut self.ecx, place.local);
}
Operand::Constant(_) => {}
}
Expand Down Expand Up @@ -973,7 +1014,7 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
}
}
}
//none of these have Operands to const-propagate
// None of these have Operands to const-propagate
TerminatorKind::Goto { .. }
| TerminatorKind::Resume
| TerminatorKind::Abort
Expand All @@ -985,8 +1026,44 @@ 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 { .. } => {}
TerminatorKind::Call { ref mut args, .. } => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should move to a general scheme where we visit_operand and mutate the operand directly instead of finding all the operands in all the terminators and statements and mutating them manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds interesting. That's probably worth a try, because I feel that we're gonna be redundant in many parts of the code after all is said and done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be specially useful for weirder Operand cases like interpret::Immediate::ScalarPair and interpret::Indirect

Copy link
Contributor Author

@felix91gr felix91gr Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, in ConstPropagator::replace_with_const https://github.com/rust-lang/rust/blob/9fc9dc99ee60a7ab603622ee83d14f8e05485740/src/librustc_mir/transform/const_prop.rs#L699 this exact thing is implemented already. There is even a comment about how Scalar and ScalarPair were relatively easy to implement compared to Operand::Indirect, and that because of that, Indirect's const prop implementation was postponed.

I'm currently trying to adapt the replace_with_const code to work with a ScalarPair that came from an Operand (still trying to work out the types, because I don't have an Rvalue to capture my results). I think I 200% second your idea now @oli-obk, we're already having code duplication and it should be straight-forward-ish to do it, provided we work out the types so that indeed we can just directly mutate an Operand without needing things like Rvalues to do 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.

@wesleywiser what do you think?

If we go ahead with this idea, I think I could do a separate PR to address it, since I feel that this requires quite some work to be done outside of mir/transform/const_prop.rs. I can work the ScalarPair cases et al there as well, since after this hypothetical function is implemented, expanding the current propagation at function callsites into ScalarPairs should be almost a one-liner.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a great idea! My only concern is that replace_with_const has some special handling for Rvalue::Aggregate and we should make sure we don't regress the optimizations we do there.

rust/src/librustc_mir/transform/const_prop.rs

Lines 724 to 730 in 82e90d6

*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),
],
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realized that I left you hanging here. Yes, indeed. I wouldn't touch whatever optimizations are in there if at all possible.

// Every argument in our function calls can be const propagated.
// We try to, before exiting.
for opr in args {
// We see if the operand can be evaluated, and if so, we continue.
if let Some(l) = opr.place().and_then(|p| p.as_local()) {
if let Some(value) = self.get_const(l) {
if self.should_const_prop(value) {
if let interpret::Operand::Immediate(
interpret::Immediate::Scalar(
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.
for local in self.locals_of_current_block.clone().iter() {
trace!(
"removing local {:?} from const-prop, since it's restricted to just its own block.",
local
);
Self::remove_const(&mut self.ecx, local);
}
// Before moving on to the next block, we must forget all restricted locals, because we
// have already removed them from the `const` pool
self.locals_of_current_block.clear();
}
}
11 changes: 11 additions & 0 deletions src/test/mir-opt/const_prop/tuple_literal_propagation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// EMIT_MIR_FOR_EACH_BIT_WIDTH

// EMIT_MIR rustc.main.ConstProp.diff
fn main() {
let x = (1, 2);

consume(x);
}

#[inline(never)]
fn consume(_: (usize, usize)) { }
69 changes: 69 additions & 0 deletions src/test/mir-opt/const_prop/tuple_literal_propagation/32bit/rustc.main.ConstProp.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
- // MIR for `main` before ConstProp
+ // MIR for `main` after ConstProp

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/tuple_literal_propagation.rs:4:11: 4:11
let _1: (usize, usize); // in scope 0 at $DIR/tuple_literal_propagation.rs:5:9: 5:10
let _2: (); // in scope 0 at $DIR/tuple_literal_propagation.rs:7:5: 7:15
let mut _3: (usize, usize); // in scope 0 at $DIR/tuple_literal_propagation.rs:7:13: 7:14
scope 1 {
debug x => _1; // in scope 1 at $DIR/tuple_literal_propagation.rs:5:9: 5:10
}

bb0: {
StorageLive(_1); // bb0[0]: scope 0 at $DIR/tuple_literal_propagation.rs:5:9: 5:10
_1 = (const 1usize, const 2usize); // bb0[1]: scope 0 at $DIR/tuple_literal_propagation.rs:5:13: 5:19
// ty::Const
// + ty: usize
// + val: Value(Scalar(0x00000001))
// mir::Constant
- // + span: $DIR/tuple_literal_propagation.rs:5:14: 5:15
+ // + span: $DIR/tuple_literal_propagation.rs:5:13: 5:19
// + literal: Const { ty: usize, val: Value(Scalar(0x00000001)) }
// ty::Const
// + ty: usize
// + val: Value(Scalar(0x00000002))
// mir::Constant
- // + span: $DIR/tuple_literal_propagation.rs:5:17: 5:18
+ // + span: $DIR/tuple_literal_propagation.rs:5:13: 5:19
// + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) }
StorageLive(_2); // bb0[2]: scope 1 at $DIR/tuple_literal_propagation.rs:7:5: 7:15
StorageLive(_3); // bb0[3]: scope 1 at $DIR/tuple_literal_propagation.rs:7:13: 7:14
- _3 = _1; // bb0[4]: scope 1 at $DIR/tuple_literal_propagation.rs:7:13: 7:14
+ _3 = (const 1usize, const 2usize); // bb0[4]: scope 1 at $DIR/tuple_literal_propagation.rs:7:13: 7:14
+ // ty::Const
+ // + ty: usize
+ // + val: Value(Scalar(0x00000001))
+ // mir::Constant
+ // + span: $DIR/tuple_literal_propagation.rs:7:13: 7:14
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00000001)) }
+ // ty::Const
+ // + ty: usize
+ // + val: Value(Scalar(0x00000002))
+ // mir::Constant
+ // + span: $DIR/tuple_literal_propagation.rs:7:13: 7:14
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) }
_2 = const consume(move _3) -> bb1; // bb0[5]: scope 1 at $DIR/tuple_literal_propagation.rs:7:5: 7:15
// ty::Const
// + ty: fn((usize, usize)) {consume}
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/tuple_literal_propagation.rs:7:5: 7:12
// + literal: Const { ty: fn((usize, usize)) {consume}, val: Value(Scalar(<ZST>)) }
}

bb1: {
StorageDead(_3); // bb1[0]: scope 1 at $DIR/tuple_literal_propagation.rs:7:14: 7:15
StorageDead(_2); // bb1[1]: scope 1 at $DIR/tuple_literal_propagation.rs:7:15: 7:16
_0 = const (); // bb1[2]: scope 0 at $DIR/tuple_literal_propagation.rs:4:11: 8:2
// ty::Const
// + ty: ()
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/tuple_literal_propagation.rs:4:11: 8:2
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_1); // bb1[3]: scope 0 at $DIR/tuple_literal_propagation.rs:8:1: 8:2
return; // bb1[4]: scope 0 at $DIR/tuple_literal_propagation.rs:8:2: 8:2
}
}

Loading