Codegen cleanups #112827
Merged
Codegen cleanups #112827
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
rustbot
added
S-waiting-on-review
Noratrieb
added a commit
to Noratrieb/rust
that referenced
this pull request
Jun 21, 2023
… r=oli-obk More codegen cleanups Some additional cleanups I found while looking closely at this code, following up from rust-lang#112827. r= `@oli-obk`
tmiasko
reviewed
Jun 21, 2023
`Message` is an enum with multiple variants, for messages sent to the coordinator thread. *Except* for `Message::CodegenItem`, which is entirely disjoint, being for messages sent from the coordinator thread to the main thread. This commit move `Message::CodegenItem` into a separate type, `CguMessage`, which makes the code much clearer.
`Message` is an enum with multiple variants. Four of those variants map directly onto the four variants of `WorkItemResult`. This commit reduces those four `Message` variants to a single variant containing a `WorkItemResult`. This requires increasing `WorkItemResult`'s visibility to `pub(crate)` visibility, but `WorkItem` and `Message` can also have their visibility reduced to `pub(crate)`. This change avoids some boilerplate enum translation code, and makes `Message` easier to understand.
This is particularly useful for `Message`.
The codegen main loop has two bools, `codegen_done` and `codegen_aborted`. There are only three valid combinations: `(false, false)`, `(true, false)`, `(true, true)`. This commit replaces them with a single tri-state enum, which makes things clearer.
nnethercote
force-pushed
the
codegen-cleanups
branch
from
6b9ad3f to
fae4f45
Compare
|
Thanks for the review, I have addressed the comments and updated the code. |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
⌛ Testing commit fae4f45 with merge 9783a638a2123efbc13657aaf1a95ede49fd1d1c... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
That's a known intermittent MIPS failure. @bors retry |
bors
added
S-waiting-on-bors
|
@bors rollup=iffy |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (fe37f37): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 663.045s -> 661.926s (-0.17%) |
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.
Some cleanups I found while looking closely at this code.
r? @tmiasko