-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backend: add infinite_register
class method to RegisterType
#3929
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3929 +/- ##
==========================================
- Coverage 91.30% 91.30% -0.01%
==========================================
Files 467 468 +1
Lines 58068 58121 +53
Branches 5581 5582 +1
==========================================
+ Hits 53021 53067 +46
- Misses 3616 3623 +7
Partials 1431 1431 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I have a few suggestions though
xdsl/dialects/x86/register.py
Outdated
@@ -146,6 +154,10 @@ def instruction_set_name(cls) -> str: | |||
def abi_index_by_name(cls) -> dict[str, int]: | |||
return SSE_INDEX_BY_NAME | |||
|
|||
@classmethod | |||
def infinite_register_name(cls, index: int): | |||
return f"sse_{index}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return f"sse_{index}" | |
return f"josse_{index}" |
xdsl/backend/register_type.py
Outdated
assert isinstance(res.index, NoneAttr), ( | ||
f"Invalid 'infinite' register name {spelling}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert isinstance(res.index, NoneAttr), ( | |
f"Invalid 'infinite' register name {spelling}" | |
) | |
assert isinstance(res.index, NoneAttr), ( | |
f"Invalid 'infinite' register name: {spelling} clashes with finite register set" | |
) |
I changed the logic to just provide the prefix, not the whole thing. One of the questions is do we want |
Not a biggie for me; leaning slightly towards using |
OK then I'll do a quick PR first doing |
7db42af
to
b941802
Compare
It's nice to have a check to make sure that our fake infinite registers don't clash with the real ones.