Refactor RegTraversalIter
#235
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
This is a non-functional change to refactor `RegTraversalIter` to make it more amenable for future commits adding limits to which registers can be allocated. This change does change the API of `RegTraversalIter`, though. It boils down to several parts: - using `Cursor` internally: much of the cursor-advancing logic is duplicated between the preferred and non-preferred register list. This change adds a new, private struct--`Cursor`--to handle this. This refactoring also reduces the scope of the borrow we take down to only what is necessary (i.e., the actual register lists). - changing parameter order: this also passes through the arguments constructing a `RegTraversalIter` in the order they will be checked during iteration: first it emits the fixed register, then the hinted register, then the offset-based lookup. This is a clarity tweak. - only allow one hint register: `RegTraversalIter` allowed for two hint registers but the library never uses the second one. This change switches to only using a single register, with no other change in behavior. - minor simplifications: e.g., we can use `Option::take` to avoid a more verbose access (and removal) of the fixed register. Another possible change here would be to pass the hinted register as an `Option<PReg>` with `None` to indicate no hint. This seems more conventional and a quick audit of the code supports this but I avoided this out of an abundance of caution.
abrown
force-pushed
the
reg-traversal-iter
branch
from
16f6604 to
10be861
Compare
cfallin
approved these changes
Sep 17, 2025
Member
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
This is much cleaner -- thanks very much!
I'm not sure why the original implementation didn't use Option<PReg> for the hint field rather than the separate flag and I agree that'd be cleaner -- feel free to make that change as well.
Member
|
(I'll merge this now but happy to take said additional cleanup as part of another PR.) |
abrown
added a commit
to abrown/regalloc2
that referenced
this pull request
Sep 18, 2025
As noted in a [comment] to bytecodealliance#235, passing an `Option<PReg>` _is_ a bit more clear API. To do this, this change adds `PReg::is_valid()` to do the conversion. This change (rather unnecessarily) also renames this variable to `hint` everywhere (previously: `hint`, `hint_reg`, `reg_hint`) and uses variables directly in several close-by `format!` strings. This is a non-functional change. [comment]: bytecodealliance#235 (review)
abrown
added a commit
to abrown/regalloc2
that referenced
this pull request
Sep 18, 2025
As noted in a [comment] to bytecodealliance#235, passing an `Option<PReg>` _is_ a bit more clear API. To do this, this change adds `PReg::is_valid()` to do the conversion. This change (rather unnecessarily) also renames this variable to `hint` everywhere (previously: `hint`, `hint_reg`, `reg_hint`) and uses variables directly in several close-by `format!` strings. This is a non-functional change. [comment]: bytecodealliance#235 (review)
cfallin
pushed a commit
that referenced
this pull request
Sep 18, 2025
As noted in a [comment] to #235, passing an `Option<PReg>` _is_ a bit more clear API. To do this, this change adds `PReg::is_valid()` to do the conversion. This change (rather unnecessarily) also renames this variable to `hint` everywhere (previously: `hint`, `hint_reg`, `reg_hint`) and uses variables directly in several close-by `format!` strings. This is a non-functional change. [comment]: #235 (review)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This is a non-functional change to refactor
RegTraversalIterto make it more amenable for future commits adding limits to which registers can be allocated. This change does change the API ofRegTraversalIter, though.It boils down to several parts:
Cursorinternally: much of the cursor-advancing logic is duplicated between the preferred and non-preferred register list. This change adds a new, private struct--Cursor--to handle this. This refactoring also reduces the scope of the borrow we take down to only what is necessary (i.e., the actual register lists).RegTraversalIterin the order they will be checked during iteration: first it emits the fixed register, then the hinted register, then the offset-based lookup. This is a clarity tweak.RegTraversalIterallowed for two hint registers but the library never uses the second one. This change switches to only using a single register, with no other change in behavior.Option::taketo avoid a more verbose access (and removal) of the fixed register.Another possible change here would be to pass the hinted register as an
Option<PReg>withNoneto indicate no hint. This seems more conventional and a quick audit of the code supports this but I avoided this out of an abundance of caution.