MIR move elimination

[Amanieu]

View all comments

This RFC proposes changes to Rust's operational semantics and MIR representation to enable elimination of unnecessary copies of local variables. Specifically, it makes accessing memory after a move undefined behavior, and redefines the allocation lifetime of local variables to be tied to their initialized state rather than their lexical scope. Finally, it introduces a new MIR optimization pass which exploits these guarantees to eliminate copies between locals when it is safe to do so.

Important

Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.

If you don't have a particular section of the RFC to comment on, you can click on the "Comment on this file" button on the top-right corner of the diff, to the right of the "Viewed" checkbox. This will create a separate thread even if others have commented on the file too.

Rendered

[NobodyXu]

For Copy-iable types, can rust mir just drop them after the last usage, given that it cannot have a Drop implementation?

It seems to be extremely strange to say I need a move keyword on a Copy-iable type just so the compiler can optimize it

[Amanieu]

For Copy-iable types, can rust mir just drop them after the last usage, given that it cannot have a Drop implementation?

It seems to be extremely strange to say I need a move keyword on a Copy-iable type just so the compiler can optimize it

We can drop Copy types after the last usage as long as they've not been borrowed. If they have then the compiler would need to additionally prove through alias analysis that the borrow has ended. This is necessary because stack/tree borrows allows a pointer/reference to continue accessing a local after it has been copied (but not moved).

The purpose of a move keyword would be to forcibly end the borrows of a local early, which allows the local to be freed at that point. This would also be enforced by the borrow checker for references.

[NobodyXu]

The purpose of a move keyword would be to forcibly end the borrows of a local early, which allows the local to be freed at that point. This would also be enforced by the borrow checker for references.

Wouldn't it make more sense to have something similar to drop to force drop it, and that can also work on non-Copy-iable type as well, for generic functions?

[Amanieu]

Calling drop(x) on a Copy type doesn't do anything since x is copied. The new keyword would allow you to write drop(move x) which forces x to be moved. Here's an example where this matters:

let x = 1;
let y = &x;
drop(move x);
let z = *y; // Fails because x was moved. Removing `move` fixes this.

Anyways, move isn't even being proposed in this RFC, it's a possible future extension.

[PoignardAzur]

Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.

[Jules-Bertholet]

Even in the case where all of the fields of a local variable have been moved out one-by-one, the local will not be freed.

Why not?

[Jules-Bertholet]

Why can't we say that accessing the moved field is UB? Because we don't have NB, the compiler can't exploit that UB by stashing another allocation with an observable address in the empty space. But that doesn't mean it can't still be UB detected by Miri, if we want users to avoid relying on it. And the compiler could even make use of the UB, to stash an allocation whose address it can prove is never observed.

[Amanieu]

There's no reason we can't do it, it's just that I am not doing so in this RFC and instead leaving to future work (I will update the future possibilities section with this). There are 2 main reasons for this:

1. Adding support for partial moves makes the opsem (and by extension Miri) much more complex since we now needs to track which bytes of a local have been moved out and become "inactive" (UB to access). What happens to the padding of a struct if one field is moved out? What happens to the discriminant of an enum like Option<T> if the Some value has been moved out and the layout is optimized (the discriminant occupied the same bytes as the value)? These are all questions that would have to be answered.

2. The proposed MIR optimization pass can't easily take advantage of this, and even if it could (while respecting address observation rules) then I expect the benefit over the existing proposed pass would be minimal. It's just not worth the extra complexity of tracking lifetimes separately for every field of a local.

[oskgo]

This makes it sound as though types with Copy would be less efficient than with the status quo, but as far as I understand this is not the case. Copy types would at worst be as efficient as with the status quo.

This sounds more like a limitation (AKA an opportunity for future work) than a drawback.

It's also not clear to me why the mere act of implementing Copy for a type would inhibit the optimization in practice. Surely code that was originally written for a non-Copy type would have an access pattern where every copy could be optimized into a move?

[Amanieu]

This makes it sound as though types with Copy would be less efficient than with the status quo, but as far as I understand this is not the case. Copy types would at worst be as efficient as with the status quo.

That is correct.

This sounds more like a limitation (AKA an opportunity for future work) than a drawback.

Sure, I can move this to the future work section.

It's also not clear to me why the mere act of implementing Copy for a type would inhibit the optimization in practice. Surely code that was originally written for a non-Copy type would have a copy pattern where every copy could be optimized into a move?

It inhibits the optimization if the value has been borrowed. A move invalidates borrows whereas a copy doesn't. I've update the text with an example.

[RalfJung]

Finally had the time for a first read over this.

I haven't yet had the chance to look at your MiniRust patch, that may answer some of the questions that came up here. But of course ideally the RFC itself is already crystal clear about the intended MR semantics. :)

View changes since this review

[RalfJung]

To make it sound to generate LLVM lifetime markers from StorageLive, we will need to do something in the opsem for StorageLive.

Reading on, you clarify this later, but the order in which you explain this is confusing.

[Amanieu]

I've kept the original ordering since for most people it's better to give an overview of the intended semantics first before diving into a more formal specification for StorageDead/StorageLive. However I have added a link to that in the section intro.

[RalfJung]

This RFC also inevitable deeply changes the semantics of assignments. There should be a section on that.

And this section is where I have the biggest disagreement with the RFC: the RFC proposes to make (de)allocation of locals an implicit side-effect of (some) place/value expressions. I think that's a problem for multiple reasons

• It breaks the property that expression evaluation can be arbitrarily reordered with each other, making it harder to reason about MIR.
• For places it's not even clear how evaluation should work. I can make guesses but the RFC is not very clear about it. (EDIT: This one is answered by the MiniRust patch.)

I would propose that instead the (de)allocation happens as part of the assignment operator. The way I think about it is that the operator performs the following steps:

• evaluate the value expression (RHS)
• deallocate a set of locals
• allocate a set of locals
• evaluate the place expression (LHS)
• store the value into the place

In MiniRust, it's probably easiest to just annotate those two sets of locals as part of the syntax of assignment itself. In MIR, we might want to make that implicit. That would then be able to capture the syntactic ruls you have proposed elsewhere:

• for the locals to deallocate: if the RHS is Use(Move(local)), then local is deallocated, otherwise nothing gets deallocated. (Or is it really all Move(local) operands on the RHS? I can see it being useful for aggregate initialization but I doubt this makes much sense for binops.)
• for the locals to allocate: if the LHS is local.non-deref-projs, then local gets allocated, otherwise nothing gets allocated

[Amanieu]

As per our discussion on Zulip, I've kept the existing semantics of doing allocation/de-allocation as part of destination place/input operand evaluation. I've added bulk allocation/deallocation as an alternative option, but in the end both options are workable for MIR optimizations, the only difference is in how they are handled in Miri/MiniRust.

[theemathas]

How does this work with unwinding (which causes multiple variables to be deallocated "at the same time") from a scope with variables that don't have destructors? See rust-lang/rust#147875

View changes since the review

[Amanieu]

It just works? The lifetime of a local on the unwind path ends either at StorageDead or at an UnwindResume/UnwindTerminate terminator.

[joshtriplett]

We happened to look at this in an @rust-lang/lang RFC review. We're currently assuming that the RFC qua RFC doesn't need attention from lang until the experiment has progressed further, and that you'll nominate it when you need review here.

[Amanieu]

Right, the current plan is to first land rust-lang/rust#157943 under the -Zmir-move-elimination flag. Then, we can start an FCP on the RFC to adopt the new local lifetime semantics and switch the optimization to be on by default.

[RalfJung]

Looks very good overall. :)
I only carefully reviewed the part relevant for MIR semantics. There's a big section on how to do the MIR optimizations that I skipped, but also I don't think the details of that section should be considered normative anyway.

View changes since this review

[Amanieu]

I implemented Miri support for the new semantics in rust-lang/rust#162048. Two details in particular stood out.

ZST handling

There are several places in MIR intrinsic lowering that assume that unit types (and in one case ZST closures) don't need to be initialized before being accessed, which is not allowed by the new semantics. This is because those ZSTs don't have an address yet (which requires an allocation).

Additionally the RemoveZsts MIR pass eliminates assignments to ZSTs since it considers them to be no-ops.

The current solution is to keep the old semantics for ZSTs by having them be fully allocated by StorageLive instead of deferring this to the point where they are initialized. An alternative solution would be to change the RemoveZsts pass to not remove ZST assignments and to fix MIR intrinsic lowering to initialize their () return place.

Temporary allocations

Evaluating operands before the destination place in assignments is implemented by creating a temporary allocation with the same type as the destination place, evaluating the full rvalue into that and then evaluating the destination place and copying the result from the temporary allocation into that. This temporary allocation shouldn't be observable in practice since there is no pointer with a valid tag to access it. However it may cause Miri to be slower (I haven't benchmarked it).

Temporary allocations are also used for moved-out locals and moved call arguments.

[Amanieu]

I've updated the RFC based on the experience from implementing Miri support for these semantics and then validating the existing MIR passes against it. Main changes are:

• Added a StorageAlloc statement that forces a local to become allocated. This is needed by some MIR passes that try to take the address of a local before it is initialized.
• Call arguments now mostly follow the same evaluation order and rules as normal operands.
• Reworked the optimization impact section based on experience.

[RalfJung]

They do not apply to TailCall, InlineAsm, Yield, which continue to use the standard operand evaluation rules.

Those rules are for statements. I have no idea what it means to apply them to terminators.

Call and TailCall having different rules sound quite confusing and is IMO undesirable.

View changes since the review

[Amanieu]

There's nothing specific about these rules that make them only apply to statements?

Call and TailCall already have different rules today because TailCall doesn't allow in-place passing. In fact of all terminators, Call is the special case because it is the only MIR statement/termiantor that treats move with special semantics to allow in-place calls. I've previously argued that we should use something other than Operand for call terminators for this reason.

[RalfJung]

So move _1 and move _1.field have completely different semantics? That doesn't sound like a good idea, it sounds like a headache and a footgun.

View changes since the review

[Amanieu]

That's already the case for normal (non-call) operand evaluation: move _1 will de-allocate the local but move _1.field is treated the same as a copy.

[bjorn3]

move *_1 is definitively treated as move, not copy (otherwise impl FnOnce() for Box<dyn FnOnce()> couldn't work). And cg_clif also treats move _1.field as move and doesn't copy it on the caller side, allowing the callee to reuse it in-place: https://github.com/rust-lang/rust/blob/7d2cd0fbc092625ea371da704f15f80216d2220e/compiler/rustc_codegen_cranelift/src/abi/mod.rs#L416-L424

[RalfJung]

That's already the case for normal (non-call) operand evaluation

Before this RFC it's not the case, it's all nice and homogeneous.

If this RFC makes things messy also for non-call operands that's not really helping.^^

[Amanieu]

The main change in this RFC is that moves of a whole local now deallocate the local. Otherwise it behaves the same as before.

	Old semantics	New semantics
Normal operand copy _1	Copies the value, source unchanged	Copies the value, source unchanged
Normal operand move _1	Same as copy	Deallocates _1 during evaluation
Normal operand move _1.field	Same as copy	Same as copy
Call operand copy _1	Copies the value, source unchanged	Copies the value, source unchanged
Call operand move _1	Protects _1 for duration of call	Deallocates _1 during evaluation
Call operand move _1.field	Protects _1.field for duration of call	Protects _1.field for duration of call

(.field in this table generalizes to any projection, including deref)

Note that both deallocation and protection permit the codegen optimization of in-place passing. Deallocation allows it because it allows the local in the callee's stack frame to re-use the newly freed address. Protection allows it because the protected area is temporarily donated for the callee to allocate its locals from. No changes to codegen were required by the change from protection to deallocation.

[Amanieu]

Deallocation also cleanly detects overlaps with other move operands and the destination as UB:

• UB if the local was already protected or deallocated by an earlier operand evaluation.
• Later attempts to protect or deallocate the place will be UB since it has been deallocated.

And finally, it deallocates moved locals before the callee executes, which means the stack frame doesn't need to keep track of a set of locals to deallocate on return.

[RalfJung]

What does "forbid" mean? Is it UB, or invalid MIR?

This all feels like it's papering over a deeper problem by adding increasingly baroque exceptions and special cases. :/

View changes since the review

[Amanieu]

Invalid MIR. It was already UB in the old semantics because of the overlap between source and destination, this just makes the guarantee stronger.

[RalfJung]

Previously UB naturally fell out of the semantics without special cases. Now there's more special cases that everyone dealing with MIR semantics needs to figure out and remember. That's an increase in entropy.

Why is it invalid MIR now?

[Amanieu]

Because without that extra rule, it would be legal to have _1 = foo(move _1) without Miri detecting any UB. Previously this would have been caught because protecting both the destination and the moved operand would result in a conflict, which is UB. Under the new semantics, _1 is freed by operand evaluation, and then re-initialized with a new allocation by destination evaluation. This works fine in the abstract machine, but breaks in-place passing in codegen.

The solution that I chose is to make the specific case where the same local is freed and the re-initialized in the destination invalid MIR specifically for calls. But there are also alternative solutions that achieve the same effect:

• Make it UB instead of invalid MIR.
• Allow it and have codegen detect this specific situation, in which case it must treat the move _1 as a copy and not pass it in-place.