Uh oh!
There was an error while loading. Please reload this page.
NLL: experiment with inverting liveness - #53314
Merged
Merged
Conversation
rust-highfive
commented
Aug 13, 2018
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 |
nikomatsakisforce-pushed
the
nll-invert-liveness
branch
from
August 13, 2018 21:58
809bc05 to
49b6773Comparenikomatsakis
commented
Aug 13, 2018
ContributorAuthor
@bors try |
bors
commented
Aug 13, 2018
Collaborator
bors
commented
Aug 14, 2018
Collaborator
☀️ Test successful - status-travis |
nikomatsakis
commented
Aug 14, 2018
ContributorAuthor
@rust-timer build 0d2e1bf |
rust-timer
commented
Aug 14, 2018
Collaborator
Success: Queued 0d2e1bf with parent d5a448b, comparison URL. |
BurntPizza
commented
Aug 14, 2018
Contributor
Looks nice, except for |
nikomatsakis
commented
Aug 14, 2018
ContributorAuthor
Curious. I suspect that is not meaningful, though, since NLL is not enabled in that configuration. Still...weird. |
nikomatsakis
commented
Aug 14, 2018
ContributorAuthor
OK, well, this seems to have enough widespread benefits to be worth landing. |
| /// A little data structure that makes it more efficient to find the | ||
| /// predecessors of each point. | ||
| crate struct PointIndexMap<'me, 'tcx> { |
ContributorAuthor
There was a problem hiding this comment.
oh, hmm, I had thought about removing this code and merging this into RegionValueElements. Maybe I should do that before landing.
| /// read or modified. e.g., `y` is used in `x = y` but not `x`. | ||
| first_use_at: IndexVec<LocalWithRegion, Option<AppearanceIndex>>, | ||
| /// Head of a linked list of **uses** of each variable -- use in |
Contributor
There was a problem hiding this comment.
this comment seems like it needs a sed -e 's/use/drop/g, no?
| /// read or modified. e.g., `y` is used in `x = y` but not `x`. | ||
| first_drop_at: IndexVec<LocalWithRegion, Option<AppearanceIndex>>, | ||
| appearances: IndexVec<AppearanceIndex, Appearance>, |
Contributor
There was a problem hiding this comment.
silly question: there is an invariant that the linked lists formed in this store of Appearances never share tails, right? At first I had thought there might be some reason that you used a single datatype for all three of the lists above, but now I'm figuring it must just be expedience, right?
ContributorAuthor
There was a problem hiding this comment.
Just expedience. Originally I thought I would have to do more forms of inter-linking -- i.e., maybe I would want to iterate not just "all places that a local is used" but also "all locals used at this place" -- but that did not turn out to be true. If that had been true, then I think a single type might have been more useful.
| // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
pnkfelix
commented
Aug 15, 2018
Contributor
my gut reaction is: "Is this somehow reinventing a dataflow analysis that we don't currently support because our dataflow infrastructure only supports forward-flows?" Maybe I'm missing something crucially different that a hypothetical generalized dataflow could not provide. (Or maybe its simpler to just write some one-off code like this for now until we have at least two analyses that require a backward flow...) |
ContributorAuthor
No, this is one iteration past that. In particular, But what this code does is to rewrite that. In particular, instead of computing the "set of live variables at a given block" we now answer the question "for which blocks is a variable live". This turns out to be a better question to answer efficiency wise (though not by that much) -- in part because the resulting bitset is exactly what our regions want as input (whereas with the older style, we had to "invert" the result). |
pnkfelix
commented
Aug 16, 2018
Contributor
Ah yes. That is a pretty important (and succinct) insight. |
nikomatsakis
commented
Aug 19, 2018
ContributorAuthor
@pnkfelix I added the comments you requested. I think I will (a) rebase and then (b) try to do one more "cleanup" pass on the code. I guess your final comment means that you at least agree with the general concept of this PR? :) |
nikomatsakisforce-pushed
the
nll-invert-liveness
branch
from
August 20, 2018 14:37
0671a53 to
4b76fe2Compare
This comment has been minimized.
This comment has been minimized.
nikomatsakisforce-pushed
the
nll-invert-liveness
branch
from
August 20, 2018 15:05
4b76fe2 to
2bd4ba5Comparepnkfelix
commented
Aug 20, 2018
Contributor
@nikomatsakis hmm I thought I had put a comment on here with an r=me once xyz is addressed. Well I'll try to double-check the commit series tomorrow to make sure I'm not mixing this up with a different PR. |
rust-highfive
commented
Aug 21, 2018
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 |
nikomatsakisforce-pushed
the
nll-invert-liveness
branch
from
August 21, 2018 19:11
4aed373 to
2a37e03Comparenikomatsakisforce-pushed
the
nll-invert-liveness
branch
from
August 27, 2018 18:01
f4e0e0c to
09feec6Comparenikomatsakis
commented
Aug 27, 2018
ContributorAuthor
@bors r=pnkfelix |
bors
commented
Aug 27, 2018
Collaborator
📌 Commit 09feec6 has been approved by |
bors
commented
Aug 28, 2018
Collaborator
⌛ Testing commit 09feec6 with merge b51bd0babd32f4e50d7ee895d9b1dded660b376d... |
bors
commented
Aug 28, 2018
Collaborator
💔 Test failed - status-travis |
rust-highfive
commented
Aug 28, 2018
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 |
which does not have as many feature gates enabled.
pnkfelix
commented
Aug 28, 2018
Contributor
@bors r+ |
bors
commented
Aug 28, 2018
Collaborator
📌 Commit 8d231ec has been approved by |
bors
commented
Aug 28, 2018
Collaborator
bors
commented
Aug 28, 2018
Collaborator
☀️ Test successful - status-appveyor, status-travis |
This was referenced Aug 28, 2018
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
6 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.
I got inspired to see what would happen here.
Fixes#52460
r? @pnkfelix