backend: add full init to RegisterType #4020
Conversation
superlopuh
requested review from
compor,
JosseVanDelm,
qaco,
jorendumoulin and
emmau678
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4020 +/- ##
==========================================
- Coverage 90.19% 90.18% -0.01%
==========================================
Files 458 458
Lines 58319 58324 +5
Branches 5688 5689 +1
==========================================
+ Hits 52598 52602 +4
Misses 4331 4331
- Partials 1390 1391 +1 ☔ View full report in Codecov by Sentry. |
| return index_attr, spelling | ||
|
|
||
| @classmethod | ||
| def from_spelling(cls, spelling: StringAttr | str) -> Self: |
There was a problem hiding this comment.
This name has been bothering me in the last few PRs, because in my mind spelling redirects to orthography.
Moreover, there is no mention in the RISC-V or Arm ISA specification manuals, so it is completely foreign.
Can't we call this simply and plainly from_name? I mean, we also have register_name at some point, and it won't affect verification even if I give an erroneous register name.
Lastly, for example, in the RISC-V case you have the register number and the register name (e.g., x8 and s0/fp).
The index will cover the register number part, while for xN this is just a mnemonic name of that, and the sN is the corresponding ABI name.
In any case, my point is that these are just names, and we should just call them that.
There was a problem hiding this comment.
Hopefully, this is just a find/replace kind of thing.
There was a problem hiding this comment.
I completely agree, with the caveat that name is already taken. Let's discuss this on Zulip.
As noted by Chris in #4020 (comment), `spelling` is not the ideal name for the concept. `name` is already taken, so I propose `register_name`. We already have a helper that returns a `str` called `register_name`, which I propose to just remove.
| @@ -5,23 +5,23 @@ | |||
|
|
|||
|
|
|||
| def test_unallocated_register(): | |||
| unallocated = x86.register.GeneralRegisterType("") | |||
| unallocated = x86.register.GeneralRegisterType.from_spelling("") | |||
There was a problem hiding this comment.
It means that, as before, the one and only difference between an allocated register and an unallocated one is that the former has a name/spelling?
There was a problem hiding this comment.
Unallocated is always "" and None for name and index, and allocated is always != "", and not None for name and index.
A checkpoint in what I've been slowly working towards, we need to have a way to explicitly create the register we need, and not just from a string.
Verifier PR to follow.