-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle discriminant in DataflowConstProp #107411
Conversation
Failed to set assignee to
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
r? wg-mir-opt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Great to have propagation of discriminants and through downcasts back again. It seems to me like this here should be sound. And limiting the tracking when creating the map is a much better fallback than what we currently have.
There is now quite some duplication due to all the _discr
methods, perhaps there is a nice way to unify this.
} | ||
} | ||
|
||
/// The target place must have been flooded before calling this method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behavior of find
followed by assign_idx
should be the same as assign
. But flooding now requires the actual place instead of just its index, so... Perhaps the easier way is to just rename assign_idx
to reflect the different behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion for the name? I'm a bit out of inspiration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like assign_idx_without_flood
. I think a long name is fine, as this is also a dangerous function to call. This also makes it obvious that DataflowConstProp has to flood before using this method when handling the checked binary operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also consider renaming assign_place_idx
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed them insert_*_idx
.
About dangerousness: #107009 interprets MIR backwards, so it needs to call these functions before flooding.
☔ The latest upstream changes (presumably #106908) made this pull request unmergeable. Please resolve the merge conflicts. |
ca17c06
to
763b8e1
Compare
763b8e1
to
a65b2a8
Compare
☔ The latest upstream changes (presumably #107601) made this pull request unmergeable. Please resolve the merge conflicts. |
a65b2a8
to
1674261
Compare
☔ The latest upstream changes (presumably #107267) made this pull request unmergeable. Please resolve the merge conflicts. |
Most of this PR should now be unnecessary now that we don't deaggregate anymore, right? |
This PR has 2 parts:
We do not ban direct assignment to variants, see the Remains the question of the semantics of a = NonZeroUsize { 0: 0_usize }
b = Option::<NonZeroUsize>::Some(a) Notably:
There are 2 ways to handle this: (b) renders part (1) of this PR unnecessary, as all such assignments will go through pointers, unsupported by this pass. I would also remove the |
1674261
to
c577ae0
Compare
☔ The latest upstream changes (presumably #107727) made this pull request unmergeable. Please resolve the merge conflicts. |
c577ae0
to
df889c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with typo fixed
@bors r+ |
…oli-obk Handle discriminant in DataflowConstProp cc `@jachris` r? `@JakobDegen` This PR attempts to extend the DataflowConstProp pass to handle propagation of discriminants. We handle this by adding 2 new variants to `TrackElem`: `TrackElem::Variant` for enum variants and `TrackElem::Discriminant` for the enum discriminant pseudo-place. The difficulty is that the enum discriminant and enum variants may alias each another. This is the issue of the `Option<NonZeroUsize>` test, which is the equivalent of rust-lang/unsafe-code-guidelines#84 with a direct write. To handle that, we generalize the flood process to flood all the potentially aliasing places. In particular: - any write to `(PLACE as Variant)`, either direct or through a projection, floods `(PLACE as OtherVariant)` for all other variants and `discriminant(PLACE)`; - `SetDiscriminant(PLACE)` floods `(PLACE as Variant)` for each variant. This implies that flooding is not hierarchical any more, and that an assignment to a non-tracked place may need to flood a tracked place. This is handled by `for_each_aliasing_place` which generalizes `preorder_invoke`. As we deaggregate enums by putting `SetDiscriminant` last, this allows to propagate the value of the discriminant. This refactor will allow to make rust-lang#107009 able to handle discriminants too.
Rollup of 7 pull requests Successful merges: - rust-lang#105300 (rework min_choice algorithm of member constraints) - rust-lang#107163 (Remove some superfluous type parameters from layout.rs.) - rust-lang#107173 (Suggest the correct array length on mismatch) - rust-lang#107411 (Handle discriminant in DataflowConstProp) - rust-lang#107968 (Enable `#[thread_local]` on armv6k-nintendo-3ds) - rust-lang#108032 (Un📦ing the Resolver) - rust-lang#108060 (Revert to using `RtlGenRandom` as a fallback) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Correctly handle aggregates in DataflowConstProp The previous implementation from rust-lang#107411 flooded target of an aggregate assignment with `Bottom`, corresponding to the `deinit` that the interpreter does. As a consequence, when assigning `target = Enum::Variant#i(...)` all the `(target as Variant#j)` were at `Bottom` while they should have been `Top`. This PR replaces that flooding with `Top`. Aside, it corrects a second bug where the wrong place would be used to assign to enum variant fields, resulting to nothing happening. Fixes rust-lang#108166
Correctly handle aggregates in DataflowConstProp The previous implementation from rust-lang#107411 flooded target of an aggregate assignment with `Bottom`, corresponding to the `deinit` that the interpreter does. As a consequence, when assigning `target = Enum::Variant#i(...)` all the `(target as Variant#j)` were at `Bottom` while they should have been `Top`. This PR replaces that flooding with `Top`. Aside, it corrects a second bug where the wrong place would be used to assign to enum variant fields, resulting to nothing happening. Fixes rust-lang#108166
cc @jachris
r? @JakobDegen
This PR attempts to extend the DataflowConstProp pass to handle propagation of discriminants. We handle this by adding 2 new variants to
TrackElem
:TrackElem::Variant
for enum variants andTrackElem::Discriminant
for the enum discriminant pseudo-place.The difficulty is that the enum discriminant and enum variants may alias each another. This is the issue of the
Option<NonZeroUsize>
test, which is the equivalent of rust-lang/unsafe-code-guidelines#84 with a direct write.To handle that, we generalize the flood process to flood all the potentially aliasing places. In particular:
(PLACE as Variant)
, either direct or through a projection, floods(PLACE as OtherVariant)
for all other variants anddiscriminant(PLACE)
;SetDiscriminant(PLACE)
floods(PLACE as Variant)
for each variant.This implies that flooding is not hierarchical any more, and that an assignment to a non-tracked place may need to flood a tracked place. This is handled by
for_each_aliasing_place
which generalizespreorder_invoke
.As we deaggregate enums by putting
SetDiscriminant
last, this allows to propagate the value of the discriminant.This refactor will allow to make #107009 able to handle discriminants too.