Fix MIR CopyPropagation errneously propagating assignments to function args#45753
Fix MIR CopyPropagation errneously propagating assignments to function args#45753bors merged 2 commits intorust-lang:masterfrom
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
7a3d939 to
a737194
Compare
|
I think it's cleaner to have the check with the rest of the destination checks at ~L85: rust/src/librustc_mir/transform/copy_prop.rs Lines 85 to 96 in 4efcc66 |
|
@arielb1 I want the copyprop to keep eliminating (I noticed I should move the check from the current weird place to just before rust/src/librustc_mir/transform/copy_prop.rs Line 121 in 4efcc66 |
|
Eliminating self-assignment is ok in even more cases (e.g. even if there are multiple definitions of the value, or multiple uses - I actually think that self-assignment can be eliminated in all cases), so I'll rather do its elimination separately. Unless for some reason we generate a lot of |
e9b171a to
226a2a3
Compare
|
Depends on #45757 |
226a2a3 to
6414371
Compare
6414371 to
1142524
Compare
|
@bors r+ |
|
📌 Commit 1142524 has been approved by |
Fix MIR CopyPropagation errneously propagating assignments to function args
Compiling this code with MIR CopyPropagation activated will result in printing `5`,
because CopyProp errneously propagates the assignment of `5` to all `x`:
```rust
fn bar(mut x: u8) {
println!("{}", x);
x = 5;
}
fn main() {
bar(123);
}
```
If a local is propagated, it will result in an ICE at trans due to an use-before-def:
```rust
fn dummy(x: u8) -> u8 { x }
fn foo(mut x: u8) {
x = dummy(x); // this will assign a local to `x`
}
```
Currently CopyProp conservatively gives up if there are multiple assignments to a local,
but it is not took into account that arguments are already assigned from the beginning.
This PR fixes the problem by preventing propagation of assignments to function arguments.
|
☀️ Test successful - status-appveyor, status-travis |
|
Thank you for reviewing. |
Fix CopyPropagation regression (2) Remaining part of MIR copyprop regression by (I think) rust-lang#45380, which I missed in rust-lang#45753. ```rust fn foo(mut x: i32) -> i32 { let y = x; x = 123; // `x` is assigned only once in MIR, but cannot be propagated to `y` y } ``` So any assignment to an argument cannot be propagated.
Compiling this code with MIR CopyPropagation activated will result in printing
5,because CopyProp errneously propagates the assignment of
5to allx:If a local is propagated, it will result in an ICE at trans due to an use-before-def:
Currently CopyProp conservatively gives up if there are multiple assignments to a local,
but it is not took into account that arguments are already assigned from the beginning.
This PR fixes the problem by preventing propagation of assignments to function arguments.