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.

[Amanieu]

I've updated the details of the MIR pass to match what is implemented in rust-lang/rust#156046. I believe this RFC is now ready for review by @rust-lang/compiler.

@rustbot review

[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.

[tmiasko]

The RFC offers no basis for the special treatment of Drop. Is it an omission?

View changes since the review

[Amanieu]

You're right, it's not justified by the RFC. I'll remove it.

In #156046 I did try using this to work around the lack of StorageDead markers in the unwind path (rust-lang/rust#147875), which caused false overlaps in local lifetimes in the unwind path.

[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

[RalfJung]

Suggested change

	Writes to places that *do* have a `Deref` projection will still require that the base local be allocated, otherwise behavior is undefined.
	Writes to places that *do* have a `Deref` projection are *reading* the base local of the place expression. They will hence still require that local to be allocated, otherwise behavior is undefined.

[RalfJung]

I seem to recall some open questions here regarding Call?
My implicit assumption is that all operands of the Call terminator must be evaluated before the function is actually invoked. So yes, the destination is evaluated last, but it is still evaluated before we push a stack frame.

If we don't do that, things get quite strange -- we'd have to store a place expression in the stack frame (in Miri/MiniRust) and delay executing that expression until after the call returns. That's at least very unusual, we don't store any expressions in the Abstract Machine state so far. The RFC should be explicit about this, and justify its choice if things are truly delayed until after the call returns.

[RalfJung]

So... the stack frame (in Miri/MiniRust) also needs to remember a list of locals to free after the function returned?

This is an odd special case. The RFC should include some sort of justification for this choice.

[RalfJung]

MIR optimizations also need to be aware that place/(r)value expressions cannot be freely reordered around each other any more as they can have side-effects.