Recognize discriminant reads as no-ops in RemoveNoopLandingPads #77793
Conversation
The cleanup blocks often contain read of discriminants. Teach RemoveNoopLandingPads to recognize them as no-ops to remove additional no-op landing pads.
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit ecd7862 with merge 847577274ba21becb5fce245fb53d7d6a392e951... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 847577274ba21becb5fce245fb53d7d6a392e951 with parent cae8bc1, future comparison URL. |
| @@ -43,7 +43,7 @@ impl RemoveNoopLandingPads { | |||
| // These are all nops in a landing pad | |||
| } | |||
|
|
|||
| StatementKind::Assign(box (place, Rvalue::Use(_))) => { | |||
| StatementKind::Assign(box (place, Rvalue::Use(_) | Rvalue::Discriminant(_))) => { | |||
| if place.as_local().is_some() { | |||
There was a problem hiding this comment.
I wonder how common it is to have writes to unprojected locals that are not drop flags along the cleanup path? Seems like we could ignore these as well.
|
LGTM, I'd like to see a MIR opt test in which these changes have an impact, although maybe this is more speculative? I'll check back in once perf results are up. |
|
Finished benchmarking try commit (847577274ba21becb5fce245fb53d7d6a392e951): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
I was examining MIR of functions making extensive use of I also experimented with a more aggressive variant, but that had no additional effect. Maybe someone more familiar with code generated by drop elaboration could tell if there is some particular pattern still missing, but I didn't notice any. Regarding tests, this pass runs twice with the same name, and diff is unfortunately for the wrong one, which is why so far I didn't add any. |
|
Got it! You could add a name to @bors delegate+ |
|
✌️ @tmiasko can now approve this pull request |
|
@bors r=ecstatic-morse |
|
Hmm, seems like delegation is broken @bors r=ecstatic-morse |
|
📌 Commit ecd7862 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
|
Excellent (2-3% instructions, <1% wall-time) improvements on several real-world benchmarks, including style-servo. Great work! |
The cleanup blocks often contain read of discriminants. Teach
RemoveNoopLandingPads to recognize them as no-ops to remove
additional no-op landing pads.