optimize promote_consts by caching the results of validate_local
#96815
Conversation
rustbot
added
the
T-compiler
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
SparrowLii
changed the title
promote_consts by cache the validate checkpromote_consts by caching the results of validate_local
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit cb7f116 with merge 200eb1b754094e0698b7b51aa7e48b0af030432b... |
|
☀️ Try build successful - checks-actions |
|
Queued 200eb1b754094e0698b7b51aa7e48b0af030432b with parent 6139205, future comparison URL. |
|
Finished benchmarking commit (200eb1b754094e0698b7b51aa7e48b0af030432b): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
rustbot
removed
the
S-waiting-on-perf
|
r? @oli-obk |
| Ok(()) => Validity::Validated, | ||
| Err(_) => Validity::Invalid, |
There was a problem hiding this comment.
Considering the Validity enum is just being converted back and forth between Result and itself, maybe we should just use Option<Result<(), Unpromotable>> instead. I usually like specialized enums, but in this case I think it's not very helpful.
There was a problem hiding this comment.
Hmm... actually, in case of Err(_), I think we may just want to replace self.temps[local] with TempState::Unpromotable.
There was a problem hiding this comment.
So... basically we don't need the Invalid variant, because we can just change the entire TempState.
That leaves us with Validated and Unknown.
Now I'm just worried that validate_rvalue and validate_call may switch from an Ok to an Unpromotable if run again. Idea: if cfg!(debug_assertions) is on, always run the Unknown path, but then, if we already had a cached Validated, check that we would have set it to Validated again and span_bug! otherwise.
There was a problem hiding this comment.
I don't think we need to worry about this.
validate_localonly checks the statement which anlocalfirst appears (and as an lvalue), so thelocalwill not appear in the rvalue or the function parameters of this statement.- During the validate_* process,
validate_localis always executed whenlocalis encountered, so their results are always the same.
There was a problem hiding this comment.
Hmm... actually, in case of
Err(_), I think we may just want to replaceself.temps[local]withTempState::Unpromotable.
That makes sense!
|
@bors r+ |
|
📌 Commit b890037 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (e013f9e): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
From the FIXME in the impl of
promote_consts. Early return thevalidate_localshould save some compile time.qualif_localis similar to this, but requires futher changing because there are different types of qualif checks. If this PR is effective, I will do it as well.