Use dataflow to propagate Qualifs during const-qualification#63860
Closed
ecstatic-morse wants to merge 8 commits intorust-lang:masterfrom
Closed
Use dataflow to propagate Qualifs during const-qualification#63860ecstatic-morse wants to merge 8 commits intorust-lang:masterfrom
Qualifs during const-qualification#63860ecstatic-morse wants to merge 8 commits intorust-lang:masterfrom
Conversation
rust-highfive
added
the
S-waiting-on-review
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
3 tasks
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
24cf5ff to
de55d91
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Collaborator
|
☔ The latest upstream changes (presumably #63580) made this pull request unmergeable. Please resolve the merge conflicts. |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
999dc59 to
ff0d211
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
ff0d211 to
c970931
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
c970931 to
2e8921a
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
44e4bf3 to
6ab8410
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
6ab8410 to
4dd29eb
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
2 times, most recently
from
eebfcce to
1ddae0b
Compare
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
ecstatic-morse
force-pushed
the
qualifs-temp
branch
2 times, most recently
from
71c1209 to
8fd7189
Compare
Centril
added a commit
to Centril/rust
that referenced
this pull request
Sep 5, 2019
Add a `Place::is_indirect` method to determine whether a `Place` contains a `Deref` projection Working on rust-lang#63860 requires tracking some property about each local. This requires differentiating `Place`s like `x` and `x.field[index]` from ones like `*x` and `*x.field`, since the first two will always access the same region of memory as `x` while the latter two may access any region of memory. This functionality is duplicated in various places across the compiler. This PR adds a helper method to `Place` which determines whether that `Place` has a `Deref` projection at any point and changes some existing code to use the new method. I've not converted `qualify_consts.rs` to use the new method, since it's not a trivial conversion and it will get replaced anyway by rust-lang#63860. There may be other potential uses besides the two I change in this PR. r? @oli-obk
Centril
added a commit
to Centril/rust
that referenced
this pull request
Sep 5, 2019
Add a `Place::is_indirect` method to determine whether a `Place` contains a `Deref` projection Working on rust-lang#63860 requires tracking some property about each local. This requires differentiating `Place`s like `x` and `x.field[index]` from ones like `*x` and `*x.field`, since the first two will always access the same region of memory as `x` while the latter two may access any region of memory. This functionality is duplicated in various places across the compiler. This PR adds a helper method to `Place` which determines whether that `Place` has a `Deref` projection at any point and changes some existing code to use the new method. I've not converted `qualify_consts.rs` to use the new method, since it's not a trivial conversion and it will get replaced anyway by rust-lang#63860. There may be other potential uses besides the two I change in this PR. r? @oli-obk
bors
added a commit
that referenced
this pull request
Sep 8, 2019
…oli-obk Refactor the `MirPass for QualifyAndPromoteConstants` This is an accumulation of drive-by commits while working on `Vec::new` as a stable `const fn`. The PR is probably easiest read commit-by-commit. r? @oli-obk cc @eddyb @ecstatic-morse -- your two PRs #63812 and #63860 respectively will conflict with this a tiny bit but it should be trivial to reintegrate your changes atop of this.
Collaborator
|
☔ The latest upstream changes (presumably #63994) made this pull request unmergeable. Please resolve the merge conflicts. |
To do dataflow-based qualification, we need to pass a reference to the dataflow state directly to the `Qualif::in_*` methods. As such, we move the `PerQualif<BitSet<Local>>` that holds the qualification state for each local to `Checker` itself and use a macro (`get_qualif`) to extract the correct `BitSet` when querying the qualifications for each local.
ecstatic-morse
force-pushed
the
qualifs-temp
branch
from
8fd7189 to
d94a901
Compare
Collaborator
|
☔ The latest upstream changes (presumably #63420) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
Author
|
This PR has been subsumed by #64470. |
bors
added a commit
that referenced
this pull request
Sep 29, 2019
… r=eddyb,oli-obk Implement dataflow-based const validation This PR adds a separate, dataflow-enabled pass that checks the bodies of `const`s, `static`s and `const fn`s for [const safety](https://github.com/rust-rfcs/const-eval/blob/master/const.md). This is based on my work in #63860, which tried to integrate into the existing pass in [`qualify_consts.rs`](https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs). However, the resulting pass was even more unwieldy than the original. Unlike its predecessor, this PR is designed to be combined with #63812 to replace the existing pass completely. The new checker lives in [`librustc_mir/transform/check_consts`](https://github.com/ecstatic-morse/rust/tree/split-promotion-and-validation/src/librustc_mir/transform/check_consts). [`qualifs.rs`](https://github.com/ecstatic-morse/rust/blob/split-promotion-and-validation/src/librustc_mir/transform/check_consts/qualifs.rs) contains small modifications to the existing `Qualif` trait and its implementors, but is mostly unchanged except for the removal of `IsNotPromotable` and `IsNotImplicitlyPromotable`, which are only necessary for promotion. [`resolver.rs`](https://github.com/ecstatic-morse/rust/blob/split-promotion-and-validation/src/librustc_mir/transform/check_consts/resolver.rs) contains the dataflow analysis used to propagate qualifs between locals. Finally, [`validation.rs`](https://github.com/ecstatic-morse/rust/blob/split-promotion-and-validation/src/librustc_mir/transform/check_consts/validation.rs) contains a refactored version of the existing [`Visitor`](https://github.com/rust-lang/rust/blob/ca3766e2e58f462a20922e42c821a37eaf0e13db/src/librustc_mir/transform/qualify_consts.rs#L1024) in `qualfy_consts.rs`. All errors have been associated with a `struct` to make [comparison with the existing pass](https://github.com/ecstatic-morse/rust/blob/1c19f2d540ca0a964900449d79a5d5181b43146d/src/librustc_mir/transform/qualify_consts.rs#L1006) simple. The existing validation logic in [`qualify_consts`](https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs) has been modified to allow it to run in parallel with the new validator. If [`use_new_validator`](https://github.com/rust-lang/rust/pull/64470/files#diff-c2552a106550d05b69d5e07612f0f812R950) is not set, the old validation will be responsible for actually generating the errors, but those errors can be compared with the ones from the new validator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Edit: See #64470 for the most recent implementation of dataflow-based const qualification.
In order to support arbitrary control-flow in consts (#49146, #52000, etc.), we need to extend the analysis that ensures const-safety to be flow-sensitive. Although a generic framework for bit-vector dataflow analysis already exists in
rustc, dataflow-based const qualification requires copying dataflow state between locals, which cannot be expressed as agen/killset (at least directly, see #62547).This PR introduces a more generic dataflow implementation which allows a consumer to define arbitrary transfer functions. This analysis is not as generic as possible: it still requires that the domain of the analysis be a bitset, while analyses like const-propagation require a more complex lattice. In a later PR, the existing bit-vector framework could be folded into this one via an adapter, since they currently share a lot of code.
One reason progress in this area has been so slow is that the current
qualify_constspass is doing many things at once. The sameVisitoris responsible for:Qualifs between locals.NeedsDrop).The goal of this PR is to separate the first task from the latter two while preserving the existing behavior of the const-checker (@eddyb is working to separate promotion in #63812). To accomplish this, we implement a dataflow analysis pass that propagates
Qualifs between consts. This analysis is run to fixpoint before the const checker validates the MIR statement-by-statement.However, promotion adds another layer of complexity. Because the const-checker is used to identify candidates for promotion, this pass must run even on non-const functions. Since promotion is viable only for temporary variables that are assigned to exactly once, there is no need to run the full dataflow-based qualif propagator (you can read @eddyb's notes in #63812 for more info). Instead, we abstract over the new
FlowSensitiveResolverand aTempOnlyResolver(not yet implemented) that is equivalent to the old qualif propagator with theQualifResolvertrait. Even when #63812 lands and theIsNotPromotablequalif is handled elsewhere, we will still need this abstraction to calculate the state ofHasMutInteriorandNeedsDropfor temps in non-const functions.r? @eddyb
Disclaimer:
Large parts of this PR may change before being merged, and fixing small style issues may not be the best use of reviewer time. However, comments on the high-level approach and the modifications to the
Qualiftrait, especially the naming ofwould be greatly appreciated.in_any_const_safe_value_of,