rustc_mir: double-check const-promotion candidates for sanity. #63812
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
bors
added a commit
that referenced
this pull request
Aug 22, 2019
[WIP] rustc_mir: double-check const-promotion candidates for sanity. Previously, const promotion involved tracking information about the value in a MIR local (or any part of the computation leading up to that value), aka "qualifs", in a quite stateful manner, which is hard to extend to arbitrary CFGs without a dataflow pass. However, the nature of the promotion we do is that it's effectively an SSA-like "tree" (or DAG, really), of assigned-once locals - which is how we can take them from the original MIR in the first place. This structure means that the subset of the MIR responsible for computing any given part of a const-promoted value is readily analyzable by walking that tree/DAG. This PR implements such an analysis in `promote_consts`, reusing the `HasMutInterior` / `NeedsDrop` computation from `qualify_consts`, but reimplementing the equivalent of `IsNotPromotable` / `IsNotImplicitlyPromotable`. Eventually we should be able to remove `IsNotPromotable` / `IsNotImplicitlyPromotable` from `qualify_consts`, which will simplify @ecstatic-morse's dataflow-based const-checking efforts. But currently this is mainly for a crater check-only run - it will compare the results from the old promotion collection and the new promotion validation and ICE if they don't match. r? @oli-obk
|
☀️ Try build successful - checks-azure |
|
@craterbot run mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
@rust-timer build 28d741e |
|
Success: Queued 28d741e with parent 201e52e, comparison URL. |
|
Finished benchmarking try commit 28d741e, comparison URL. |
|
Doesn't look like this is significant-enough perf regression, so we could probably even land it in compare mode. |
3 tasks
RalfJung
reviewed
Aug 26, 2019
|
@craterbot crates=full |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
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.
|
@craterbot abort |
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #63420) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
bors
added
S-waiting-on-review
Same as in another PR... @bors retry |
bors
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-author
|
@bors r=oli-obk |
|
📌 Commit f2c8628 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Oct 26, 2019
rustc_mir: double-check const-promotion candidates for sanity. Previously, const promotion involved tracking information about the value in a MIR local (or any part of the computation leading up to that value), aka "qualifs", in a quite stateful manner, which is hard to extend to arbitrary CFGs without a dataflow pass. However, the nature of the promotion we do is that it's effectively an SSA-like "tree" (or DAG, really), of assigned-once locals - which is how we can take them from the original MIR in the first place. This structure means that the subset of the MIR responsible for computing any given part of a const-promoted value is readily analyzable by walking that tree/DAG. This PR implements such an analysis in `promote_consts`, reusing the `HasMutInterior` / `NeedsDrop` computation from `qualify_consts`, but reimplementing the equivalent of `IsNotPromotable` / `IsNotImplicitlyPromotable`. Eventually we should be able to remove `IsNotPromotable` / `IsNotImplicitlyPromotable` from `qualify_consts`, which will simplify @ecstatic-morse's dataflow-based const-checking efforts. But currently this is mainly for a crater check-only run - it will compare the results from the old promotion collection and the new promotion validation and ICE if they don't match. r? @oli-obk
|
☀️ Test successful - checks-azure |
This was referenced Oct 26, 2019
ecstatic-morse
added a commit
to ecstatic-morse/rust
that referenced
this pull request
Oct 26, 2019
This is a relic from earlier attempts at dataflow-based const validation that attempted to do promotion at the same time. rust-lang#63812 takes a different approach: `IsNotPromotable` is no longer a `Qualif` and is computed lazily instead of eagerly. As a result, there's no need for an eager `TempPromotionResolver`, and we can use the only implementer of `QualifResolver` directly instead of through a trait.
4 tasks
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 27, 2019
…r=eddyb Clean up `check_consts` now that new promotion pass is implemented `check_consts::resolver` contained a layer of abstraction (`QualifResolver`) to allow the existing, eager style of qualif propagation to work with either a dataflow results cursor or by applying the transfer function directly (if dataflow was not needed e.g. for promotion). However, rust-lang#63812 uses a different, lazy paradigm for checking promotability, which makes this unnecessary. This PR cleans up `check_consts::validation` to use `FlowSensitiveResolver` directly, instead of through the now obselete `QualifResolver` API. Also, this contains a few commits (the first four) that address some FIXMEs in rust-lang#63812 regarding code duplication. They could be split out, but I think they will be relatively noncontroversial? Notably, `validation::Mode` is renamed to `ConstKind` and used in `promote_consts` to denote what kind of item we are in. This is best reviewed commit-by-commit and is low priority. r? @eddyb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, const promotion involved tracking information about the value in a MIR local (or any part of the computation leading up to that value), aka "qualifs", in a quite stateful manner, which is hard to extend to arbitrary CFGs without a dataflow pass.
However, the nature of the promotion we do is that it's effectively an SSA-like "tree" (or DAG, really), of assigned-once locals - which is how we can take them from the original MIR in the first place.
This structure means that the subset of the MIR responsible for computing any given part of a const-promoted value is readily analyzable by walking that tree/DAG.
This PR implements such an analysis in
promote_consts, reusing theHasMutInterior/NeedsDropcomputation fromqualify_consts, but reimplementing the equivalent ofIsNotPromotable/IsNotImplicitlyPromotable.Eventually we should be able to remove
IsNotPromotable/IsNotImplicitlyPromotablefromqualify_consts, which will simplify @ecstatic-morse's dataflow-based const-checking efforts.But currently this is mainly for a crater check-only run - it will compare the results from the old promotion collection and the new promotion validation and ICE if they don't match.
r? @oli-obk