Clarify semantics of `SetDiscriminant` and change enum deaggregation to match those semantics #94590

JakobDegen · 2022-03-04T01:35:45Z

This PR clarifies the semantics of the SetDiscriminant MIR statement. Quoting from the updated documentation:

If place is an ADT, this corresponds to the Rust code place = Variant(uninit, uninit, uninit). If each of the fields is initialized after the SetDiscriminant, the place is
completely initialized. Of course, you cannot initialize the fields first, as
SetDiscriminant invalidates them.

...

If place is a generator, then this invalidates only those fields which are not also
present in another variant. Those fields that are present in another variant are unchanged.

Benefits of this change

The MaybeLiveLocals analysis was previously somewhat broken in that it marked locals killed on SetDiscriminant, although this was obviously not true for any fields that had been set previously. This change makes that decision completely justified, without requiring any changes to the code.
Deaggregation of enums is now unambiguously non-lossy. Previously, deaggregation of enums would set both the fields and the discriminant - however, under some interpretations of the semantics of SetDiscriminant this would be lossy, since now possibly padding has to be preserved. With the new semantics, that is clearly and unambiguously not the case.
In a future PR, we can start emitting SetDiscriminant for structs as well, with the same semantics. This will have the benefit of making deaggregation for structs also non-lossy.

This PR does not yet "weaponize" this change by changing codegen to make use of this information. Since the old version of SetDiscriminant is a valid implementation of the new version, the existing codegen should not have to change.

This PR

The changes in code and documentation are made in the first and second commit. This causes a couple mir-opt tests to break. The broken tests can be divided into three categories, which are addressed in commits 3, 4, and 5:

Tests that were broken in the expected trivial way. All of these tests just had a SetDiscriminant statement move a couple lines up.
Const prop used to optimize the last statement in
```
(p as Variant).field = ...;
SetDiscriminant(p) = Variant;
q = discriminant(p);
```
However, since the statements have been re-ordered, it can no longer do this optimization. Commit 4 blesses the MIR opt test that this broke.
Finally, the SimplifyArmIdentity pass relied on the old ordering and no longer works with this change. However, I have already put up Fix SimplifyArmIdentity MIR opt #94177 , which re-writes this pass completely. That will also need to be adjusted after this PR, but I don't think it makes much sense to try and fix the old version of this pass, especially given that it does not run. Because of that, I have disabled the tests that were affected. I will be able to re-enable them in Fix SimplifyArmIdentity MIR opt #94177 . I could also attach that PR to this one, but I feel like these are each big enough as is.

I'm not sure if this requires a FCP from T-Compiler for the MIR semantics change. I'll leave the decision of who to cc and what the right procedure is up to the reviewer.

…ed by deaggregation

rust-highfive · 2022-03-04T01:35:48Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_mir_transform/src/prop_disc.rs

compiler/rustc_middle/src/mir/mod.rs

src/test/mir-opt/issue-73223.rs

nagisa · 2022-03-05T18:12:26Z

cc @rust-lang/wg-mir-opt

JakobDegen · 2022-03-05T20:58:28Z

Last two commits are gone, CI is going to fail but I can fix that later depending on the results of perf. @nagisa can you start the run?

tmiasko · 2022-03-05T21:08:31Z

@bors try @rust-timer queue

rust-timer · 2022-03-05T21:08:32Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-03-05T21:08:39Z

⌛ Trying commit 14f6cfb9d9b0b61cb8c9013b28b654a857f3afac with merge c055b10fc96f5cb03cd95d0257c65ca194dcc278...

compiler/rustc_middle/src/mir/mod.rs

bors · 2022-03-05T22:42:03Z

☀️ Try build successful - checks-actions
Build commit: c055b10fc96f5cb03cd95d0257c65ca194dcc278 (c055b10fc96f5cb03cd95d0257c65ca194dcc278)

rust-timer · 2022-03-05T22:42:04Z

Queued c055b10fc96f5cb03cd95d0257c65ca194dcc278 with parent ab2bd41, future comparison URL.

rust-timer · 2022-03-06T00:46:34Z

Finished benchmarking commit (c055b10fc96f5cb03cd95d0257c65ca194dcc278): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

…ore precisely. The changed semantics also make one analysis work better as written, so we remove the documentation in it that is now out of date.

…l way.

This test had some regressions due to the change in enum deaggregation. For now we accept these, as they do not show up on perf, and hope that future improvements can recover the previous result.

All of these tests are directly or indirectly broken by the `simplify-arm` and `simplify-branch` optimizations no longer firing. Unfortunately, this is a non-trivial thing to fix. Furthermore, there is an existing open PR that completely re-writes the `simplify-arm` optimization. As such, it doesn't make sense to try and fix the soon-to-be-replaced pass.

JakobDegen · 2022-03-06T03:05:15Z

Extra optimization pass is gone. One test blessed with a note that improvements to ConstProp should fix it. I've also pointed out in the documentation that there is a difference between ADTs and generators, and that SetDiscriminant potentially applies to both.

JakobDegen · 2022-03-08T04:53:16Z

The additional commit modifies const eval to implement these changes, and adds a test that detects UB which we did not detect before.

compiler/rustc_const_eval/src/interpret/place.rs

src/test/ui/consts/const-eval/ub-enum-overwrite.rs

JakobDegen · 2022-03-13T06:23:09Z

I believe the remaining concerns here have been addressed. I brought the issue about the difference between ADTs and generators up on Zulip, where I suggested that in the future we replace SetDiscriminant on generators with a .discriminant field projection. If this is still too great a concern, we can delay this change until then

nagisa · 2022-03-20T14:59:10Z

src/test/codegen/try_identity.rs

@@ -14,7 +14,8 @@ type R = Result<u64, i32>;
 #[no_mangle]
 pub fn try_identity(x: R) -> R {
 // CHECK: start:
-// CHECK-NOT: br {{.*}}
+// FIXME(JakobDegen CHECK-NOT): br {{.*}} . This test was broken by changes to enum deaggregation,


All of these should get an issue filed, though more generally this test in particular seems quite awkward to me, given that it relies on -Zunsound-mir-opts...

bors · 2022-03-31T07:49:57Z

☔ The latest upstream changes (presumably #95506) made this pull request unmergeable. Please resolve the merge conflicts.

JakobDegen · 2022-04-01T02:15:25Z

Closing in favor of something like #95125

…li-obk Add new `Deinit` statement This rvalue replaces `SetDiscriminant` for ADTs. This PR is an alternative to rust-lang#94590 , which only specifies that the behavior of `SetDiscriminant` is the same as what this rvalue would do. The motivation for this change are discussed in that PR and [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/SetDiscriminant.20and.20aggregate.20initialization.20.2394590) r? `@oli-obk`

Change the ordering of SetDiscriminant and field assignments produc…

ebac049

…ed by deaggregation

rust-highfive assigned nagisa Mar 4, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2022

nagisa reviewed Mar 5, 2022

View reviewed changes

compiler/rustc_mir_transform/src/prop_disc.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

src/test/mir-opt/issue-73223.rs Show resolved Hide resolved

JakobDegen force-pushed the change-enum-deaggregate branch from 17ae1b0 to 14f6cfb Compare March 5, 2022 20:57

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 5, 2022

tmiasko reviewed Mar 5, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2022

JakobDegen added 4 commits March 5, 2022 21:21

Adjust documentation of SetDiscriminant to describe the semantics m…

0a39e3c

…ore precisely. The changed semantics also make one analysis work better as written, so we remove the documentation in it that is now out of date.

Fix tests that were broken by the change to deaggregation in a trivia…

8a153cc

…l way.

Bless separate_const_switch MIR opt test.

b0b4212

This test had some regressions due to the change in enum deaggregation. For now we accept these, as they do not show up on perf, and hope that future improvements can recover the previous result.

JakobDegen force-pushed the change-enum-deaggregate branch from 14f6cfb to 1cdeb7f Compare March 6, 2022 03:02

JakobDegen mentioned this pull request Mar 6, 2022

Fix SimplifyArmIdentity MIR opt #94177

Closed

RalfJung reviewed Mar 8, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/place.rs Show resolved Hide resolved

RalfJung reviewed Mar 8, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/place.rs Show resolved Hide resolved

RalfJung reviewed Mar 8, 2022

View reviewed changes

src/test/ui/consts/const-eval/ub-enum-overwrite.rs Outdated Show resolved Hide resolved

Modify const eval to implement the new semantics for SetDiscriminant.

27c4f88

JakobDegen force-pushed the change-enum-deaggregate branch from ba71d97 to 27c4f88 Compare March 13, 2022 05:38

JakobDegen mentioned this pull request Mar 19, 2022

Add new Deinit statement #95125

Merged

nagisa reviewed Mar 20, 2022

View reviewed changes

JakobDegen closed this Apr 1, 2022

JakobDegen deleted the change-enum-deaggregate branch April 11, 2022 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify semantics of `SetDiscriminant` and change enum deaggregation to match those semantics #94590

Clarify semantics of `SetDiscriminant` and change enum deaggregation to match those semantics #94590

JakobDegen commented Mar 4, 2022 •

edited

Loading

rust-highfive commented Mar 4, 2022

nagisa commented Mar 5, 2022

JakobDegen commented Mar 5, 2022

tmiasko commented Mar 5, 2022

rust-timer commented Mar 5, 2022

bors commented Mar 5, 2022

This comment has been minimized.

bors commented Mar 5, 2022

rust-timer commented Mar 5, 2022

rust-timer commented Mar 6, 2022

JakobDegen commented Mar 6, 2022

JakobDegen commented Mar 8, 2022

JakobDegen commented Mar 13, 2022

nagisa Mar 20, 2022

bors commented Mar 31, 2022

JakobDegen commented Apr 1, 2022

Clarify semantics of SetDiscriminant and change enum deaggregation to match those semantics #94590

Clarify semantics of SetDiscriminant and change enum deaggregation to match those semantics #94590

Conversation

JakobDegen commented Mar 4, 2022 • edited Loading

Benefits of this change

This PR

rust-highfive commented Mar 4, 2022

nagisa commented Mar 5, 2022

JakobDegen commented Mar 5, 2022

tmiasko commented Mar 5, 2022

rust-timer commented Mar 5, 2022

bors commented Mar 5, 2022

This comment has been minimized.

bors commented Mar 5, 2022

rust-timer commented Mar 5, 2022

rust-timer commented Mar 6, 2022

JakobDegen commented Mar 6, 2022

JakobDegen commented Mar 8, 2022

JakobDegen commented Mar 13, 2022

nagisa Mar 20, 2022

Choose a reason for hiding this comment

bors commented Mar 31, 2022

JakobDegen commented Apr 1, 2022

Clarify semantics of `SetDiscriminant` and change enum deaggregation to match those semantics #94590

Clarify semantics of `SetDiscriminant` and change enum deaggregation to match those semantics #94590

JakobDegen commented Mar 4, 2022 •

edited

Loading