introduce dirty list to liveness, eliminate ins vector
#51896
Merged
Conversation
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
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit 58ee87d with merge c27e6f7936761b7b1ff3b3db58ceb9d7fec0cf34... |
|
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 |
|
☀️ Test successful - status-travis |
|
Perf is queued. |
|
(I'm updating the broken tests, those are nothing serious, just waiting for the build on my local machine) |
|
Perf URLs will be: |
|
cc #51819 |
kennytm
added
the
S-waiting-on-perf
|
☔ The latest upstream changes (presumably #51869) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
force-pushed
the
nll-liveness-dirty-list
branch
from
f140bca to
3f6fe5f
Compare
|
Rebased. Perf results are ready and look good. |
|
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 |
nikomatsakis
force-pushed
the
nll-liveness-dirty-list
branch
from
3f6fe5f to
73f8333
Compare
|
cc @rust-lang/compiler — this would be nice to land for NLL (it's a modest gain, but a useful one, and there are several other PRs building on this code). If anybody is able to review, would be appreciated. |
Zoxc
reviewed
Jul 1, 2018
src/librustc_mir/util/liveness.rs
Outdated
| // bits were not already present, then enqueue the predecessor | ||
| // as dirty. | ||
| for &pred_bb in &predecessors[bb] { | ||
| if outs[pred_bb].union(&bits) { |
There was a problem hiding this comment.
I think there should be a comment indicating that union returns true if it changes anything. I don't normally associate that with the union operator.
There was a problem hiding this comment.
Yeah, those methods are undercommented. I'll add some comments. =)
Zoxc
reviewed
Jul 1, 2018
src/librustc_mir/util/liveness.rs
Outdated
| bits.overwrite(&outs[bb]); | ||
| def_use[bb].apply(&mut bits); | ||
|
|
||
| // add `bits` to the out set for each predecessor; if those |
There was a problem hiding this comment.
Maybe add a remark that bits now contains the state at the entry point the block (and has the value the previous ins variable helt).
Zoxc
reviewed
Jul 1, 2018
| @@ -438,7 +432,6 @@ pub fn write_mir_fn<'a, 'tcx>( | |||
| .collect(); | |||
| writeln!(w, "{} {{{}}}", prefix, live.join(", ")) | |||
| }; | |||
| print(w, " ", &result.ins)?; | |||
There was a problem hiding this comment.
The removal of this is unfortunate. I don't know if anyone but me have used this for debugging though. It shouldn't be too hard to add code to compute ins on demand if needed though.
There was a problem hiding this comment.
We already have the code to recompute it -- and in fact in the MIR dump, we include those results. I could update this code to match I suppose.
There was a problem hiding this comment.
(That is, in the NLL MIR dump)
|
r=me with the comments added |
|
@bors r=Zoxc I didn't modify the dumping code because I am running short of time. Easy enough to do in the future. |
|
📌 Commit 78ea952 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
@bors p=1 Bumping priority as a number of NLL PRs are blocked on this, and NLL perf itself is pretty high priority and time sensitive. |
bors
added a commit
that referenced
this pull request
Jul 2, 2018
introduce dirty list to liveness, eliminate `ins` vector At least in my measurements, this seems to knock much of the liveness computation off the profile. r? @Zoxc cc @nnethercote
|
☀️ Test successful - status-appveyor, status-travis |
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.
At least in my measurements, this seems to knock much of the liveness computation off the profile.
r? @Zoxc
cc @nnethercote