Uh oh!
There was an error while loading. Please reload this page.
rustc_mir: Hide initial block state when defining transfer functions - #61787
Merged
Conversation
rust-highfive
commented
Jun 12, 2019
Contributor
(rust_highfive has picked a reviewer for you, use r? to override) |
Centril
commented
Jun 12, 2019
Contributor
Centril
reviewed
Jun 12, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ecstatic-morseforce-pushed
the
dataflow-split-block-sets
branch
from
June 12, 2019 23:29
dbc8b07 to
0d29e43Comparebors
commented
Jun 14, 2019
Collaborator
☔ The latest upstream changes (presumably #61817) made this pull request unmergeable. Please resolve the merge conflicts. |
ecstatic-morseforce-pushed
the
dataflow-split-block-sets
branch
from
June 14, 2019 22:22
0d29e43 to
c61b4c9Compareeddyb
commented
Jun 16, 2019
Contributor
Centril
commented
Jun 16, 2019
Contributor
I also don't know what to call things but "const-correctness" does seem confusing from a C/C++ POV if it isn't that? |
eddyb
commented
Jun 16, 2019
Contributor
To be clear: I don't think we should be using "const-correctness" to talk about mutability in Rust code. |
RalfJung
commented
Jun 16, 2019
Member
"refactor for more precise mutability information"? |
ecstatic-morse
commented
Jun 16, 2019
ContributorAuthor
Yes, I meant "const-correctness" in the C++ sense. "Ensure |
ecstatic-morse
commented
Jun 16, 2019
ContributorAuthor
I changed the title and made the names of the fields of |
bors
commented
Jun 18, 2019
Collaborator
☔ The latest upstream changes (presumably #61891) made this pull request unmergeable. Please resolve the merge conflicts. |
pnkfelix
commented
Jun 21, 2019
Contributor
@ecstatic-morse : Lets rebase this on top of PR ##62010 and then revise it to change the dataflow API to not pass entry-set at all. Does that sound okay to you? |
ContributorAuthor
ecstatic-morseforce-pushed
the
dataflow-split-block-sets
branch
from
June 21, 2019 19:41
83dd5c9 to
f9a6037Compareecstatic-morse
commented
Jun 21, 2019
ContributorAuthor
@pnkfelix. This PR now removes See the summary at the top for a full description. |
rust-highfive
commented
Jun 21, 2019
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-morseforce-pushed
the
dataflow-split-block-sets
branch
from
June 21, 2019 20:06
f9a6037 to
39faf52CompareThis commit makes `sets.on_entry` inaccessible in
`{before_,}{statement,terminator}_effect`. This field was meant to allow
implementors of `BitDenotation` to access the initial state for each
block (optionally with the effect of all previous statements applied via
`accumulates_intrablock_state`) while defining transfer functions.
However, the ability to set the initial value for the entry set of each
basic block (except for START_BLOCK) no longer exists. As a result, this
functionality is mostly useless, and when it *was* used it was used
erroneously (see rust-lang#62007).
Since `on_entry` is now useless, we can also remove `BlockSets`, which
held the `gen`, `kill`, and `on_entry` bitvectors and replace it with a
`GenKill` struct. Variables of this type are called `trans` since they
represent a transfer function. `GenKill`s are stored contiguously in
`AllSets`, which reduces the number of bounds checks and may improve
cache performance: one is almost never accessed without the other.
Replacing `BlockSets` with `GenKill` allows us to define some new helper
functions which streamline dataflow iteration and the
dataflow-at-location APIs. Notably, `state_for_location` used a subtle
side-effect of the `kill`/`kill_all` setters to apply the transfer
function, and could be incorrect if a transfer function depended on
effects of previous statements in the block on `gen_set`.Since the value of `InitialFlow` defines the semantics of the `join` operation, there's no reason to have seperate traits for each. We can add a default impl of `join` which branches based on `BOTTOM_VALUE`. This should get optimized away.
ecstatic-morseforce-pushed
the
dataflow-split-block-sets
branch
from
June 22, 2019 17:19
39faf52 to
c8cbd4fCompareecstatic-morse
commented
Jun 22, 2019
ContributorAuthor
@pnkfelix This is now rebased and ready for another round of review. |
| fn bottom_value() -> bool; | ||
| /// | ||
| /// `BottomValue` determines whether the initial entry set for each basic block is empty or full. | ||
| /// This also determines the semantics of the lattice `join` operator used to merge dataflow |
pnkfelix
commented
Jun 24, 2019
Contributor
@bors r+ |
bors
commented
Jun 24, 2019
Collaborator
📌 Commit c8cbd4f has been approved by |
bors
commented
Jun 24, 2019
Collaborator
⌛ Testing commit c8cbd4f with merge c873491e73405ff647abd78f06db6e699bafcdb0... |
bors
commented
Jun 24, 2019
Collaborator
💔 Test failed - checks-travis |
rust-highfive
commented
Jun 24, 2019
Contributor
Your PR failed on Travis (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 |
tesuji
commented
Jun 24, 2019
Contributor
Spurious failure. |
Centril
commented
Jun 24, 2019
Contributor
@bors retry spurious network |
bors
commented
Jun 24, 2019
Collaborator
bors added a commit
that referenced
this pull request
Jun 24, 2019
…kfelix rustc_mir: Hide initial block state when defining transfer functions This PR addresses [this FIXME](https://github.com/rust-lang/rust/blob/2887008e0ce0824be4e0e9562c22ea397b165c97/src/librustc_mir/dataflow/mod.rs#L594-L596). This makes `sets.on_entry` inaccessible in `{before_,}{statement,terminator}_effect`. This field was meant to allow implementors of `BitDenotation` to access the initial state for each block (optionally with the effect of all previous statements applied via `accumulates_intrablock_state`) while defining transfer functions. However, the ability to set the initial value for the entry set of each basic block (except for START_BLOCK) no longer exists. As a result, this functionality is mostly useless, and when it *was* used it was used erroneously (see #62007). Since `on_entry` is now useless, we can also remove `BlockSets`, which held the `gen`, `kill`, and `on_entry` bitvectors and replace it with a `GenKill` struct. Variables of this type are called `trans` since they represent a transfer function. `GenKill`s are stored contiguously in `AllSets`, which reduces the number of bounds checks and may improve cache performance: one is almost never accessed without the other. Replacing `BlockSets` with `GenKill` allows us to define some new helper functions which streamline dataflow iteration and the dataflow-at-location APIs. Notably, `state_for_location` used a subtle side-effect of the `kill`/`kill_all` setters to apply the transfer function, and could be incorrect if a transfer function depended on effects of previous statements in the block on `gen_set`. Additionally, this PR merges `BitSetOperator` and `InitialFlow` into one trait. Since the value of `InitialFlow` defines the semantics of the `join` operation, there's no reason to have seperate traits for each. We can add a default impl of `join` which branches based on `BOTTOM_VALUE`. This should get optimized away.
bors
commented
Jun 24, 2019
Collaborator
☀️ Test successful - checks-travis, status-appveyor |
This was referenced Jun 24, 2019
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
9 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.
This PR addresses this FIXME.
This makes
sets.on_entryinaccessible in{before_,}{statement,terminator}_effect. This field was meant to allow implementors ofBitDenotationto access the initial state for each block (optionally with the effect of all previous statements applied viaaccumulates_intrablock_state) while defining transfer functions. However, the ability to set the initial value for the entry set of each basic block (except for START_BLOCK) no longer exists. As a result, this functionality is mostly useless, and when it was used it was used erroneously (see #62007).Since
on_entryis now useless, we can also removeBlockSets, which held thegen,kill, andon_entrybitvectors and replace it with aGenKillstruct. Variables of this type are calledtranssince they represent a transfer function.GenKills are stored contiguously inAllSets, which reduces the number of bounds checks and may improve cache performance: one is almost never accessed without the other.Replacing
BlockSetswithGenKillallows us to define some new helper functions which streamline dataflow iteration and the dataflow-at-location APIs. Notably,state_for_locationused a subtle side-effect of thekill/kill_allsetters to apply the transfer function, and could be incorrect if a transfer function depended on effects of previous statements in the block ongen_set.Additionally, this PR merges
BitSetOperatorandInitialFlowinto one trait. Since the value ofInitialFlowdefines the semantics of thejoinoperation, there's no reason to have seperate traits for each. We can add a default impl ofjoinwhich branches based onBOTTOM_VALUE. This should get optimized away.