dialects: (riscv) remove Register class #1281

superlopuh · 2023-07-15T07:41:52Z

When I initially wrote the register types, the reasoning in my mind was that I wanted an optional register name, and Data cannot accept an optional generic parameter. But actually an empty string seems more appropriate than None anyway. Now that we have floats in riscv, and some duplication, it feels like a good idea to simplify. This also includes a rename of RegisterType to RegisterAttr, since we don't exclusively use them as types.

After some discussions with @kingiler on the changes in #1269, I thought I'd play around with the code to see how to make the generics work for both Register/FloatRegister and RegisterType/FloatRegisterType. There were always some issues with it locally, and quite a lot of complexity due to the extra step of indirection. This is what led me to the changes in this PR, which I propose as an alternative to #1269. It's probably a good idea for reviewers to take a look at both to have a better view of multiple potential ways forward.

codecov · 2023-07-15T07:44:28Z

Codecov Report

Patch coverage: 82.89% and project coverage change: -0.01 ⚠️

Comparison is base (ec75bbd) 89.50% compared to head (b53503b) 89.50%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1281      +/-   ##
==========================================
- Coverage   89.50%   89.50%   -0.01%     
==========================================
  Files         184      184              
  Lines       24270    24250      -20     
  Branches     3682     3676       -6     
==========================================
- Hits        21722    21704      -18     
+ Misses       1967     1966       -1     
+ Partials      581      580       -1

Impacted Files	Coverage Δ
...dsl/backend/riscv/lowering/riscv_arith_lowering.py	`98.29% <ø> (ø)`
xdsl/dialects/riscv.py	`83.70% <80.00%> (-0.14%)`	⬇️
tests/dialects/test_riscv.py	`100.00% <100.00%> (ø)`
tests/interpreters/test_riscv_func_interpreter.py	`100.00% <100.00%> (ø)`
xdsl/backend/riscv/register_allocation.py	`91.20% <100.00%> (ø)`
xdsl/dialects/riscv_func.py	`78.12% <100.00%> (ø)`
xdsl/dialects/snitch.py	`94.00% <100.00%> (ø)`
xdsl/transforms/lower_riscv_func.py	`97.43% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

xdsl/dialects/riscv.py

kingiler · 2023-07-15T09:46:57Z

xdsl/dialects/riscv.py

    immediate: AnyIntegerAttr | LabelAttr = attr_def(AnyIntegerAttr | LabelAttr)

    def __init__(
        self,
        immediate: int | AnyIntegerAttr | str | LabelAttr,
        *,
-        rd: RegisterType | Register | None = None,
+        rd: RegisterAttr | str | None = None,


Use of str may lost errors that can catch by type checkers in previous implementation. Can you revert it?

I still don't see the benefit of the type checking here.

If a client wants to give a bogus string then so be it, I don't see the problem. Almost all the time they're expected to use one of the RegisterAttr instances in Registers or None. I'd rather remove str than add back Register TBH, but I also don't see it as a serious potential risk.

BTW this is pretty much equivalent to allowing int instead of an IntegerAttr, there can be type mismatches, but we allow it for convenience's sake

I don't think so. At least IntegerAttr has a verifier.

So does this class

I just went over the codebase and the one location where this matters is xdsl/transforms/lower_riscv_func.py:

ops.append( riscv.MVOp( arg, - rd=riscv.Register(f"a{i}"), + rd=f"a{i}", ) )

as well as similar changes in this very same file. Looking into the implementation of Register, I see that Register also did not verify the name of the Register that was passed in its initializer. Hence, the current change seems to just implement what was already there. If there is a more typesafe way to write lower_riscv_func.py, we could explore that in the future, but I would suggest to remain permissive for now. In the spirit of reducing feature-creep in our PRs, maybe lets get this one in and then get the custom printing in next?

kingiler · 2023-07-15T09:47:08Z

xdsl/dialects/riscv.py

@@ -550,15 +531,15 @@ def __init__(
        self,
        immediate: int | AnyIntegerAttr | str | LabelAttr,
        *,
-        rd: RegisterType | Register | None = None,
+        rd: RegisterAttr | str | None = None,


Same as here.

kingiler · 2023-07-15T09:47:18Z

xdsl/dialects/riscv.py

@@ -636,7 +617,7 @@ def __init__(
        rs1: Operation | SSAValue,
        immediate: int | AnyIntegerAttr | str | LabelAttr,
        *,
-        rd: RegisterType | Register | None = None,
+        rd: RegisterAttr | str | None = None,


compor

I like this better:

unallocated is exposed in the API and the internal representation can be anything
it's shorter to instantiate
and more intuitive, since the type is an attribute

xdsl/dialects/riscv.py

AntonLydike

LGTM overall, just not sure the name change from type to attr is necessary?

xdsl/dialects/riscv.py

kingiler

LGTM

xdsl/dialects/riscv.py

kingiler

At least IntergerAttr has a verifier.

xdsl/dialects/riscv.py

tobiasgrosser · 2023-07-17T20:20:26Z

@superlopuh @kingiler and all, this looks very nice. I did a round of reviews and hope these help us to move forward with this patch (without sacrificing code quality).

tobiasgrosser

This LGTM. @superlopuh could you address my comments if that feels right to you and @kingiler could you check if after the comments have been addressed this PR is good-to-go?

superlopuh · 2023-07-18T06:45:09Z

@kingiler I've addressed all the comments, would be good to merge this soon. Are there any other changes that you think I should make before merging?

kingiler · 2023-07-18T08:11:19Z

LGTM

tobiasgrosser · 2023-07-18T08:20:30Z

Thank you all.

tobiasgrosser · 2023-07-18T08:21:33Z

Thank you all.

superlopuh added 8 commits July 15, 2023 13:28

RegisterType -> RegisterAttr and move register dicts

baeb382

unallocated register is an empty string not None

728590f

remove Register

150b62d

add generic RSICVRegisterAttr

597f111

FloatRegisterType -> FloatRegisterAttr

3abbca2

print unexpected arg

ad50fc0

add unallocated property

04784b1

cache unallocated class var

dc09ff0

superlopuh added the dialects Changes on the dialects label Jul 15, 2023

superlopuh requested review from nazavode, compor, JosseVanDelm, kingiler and adutilleul July 15, 2023 07:41

superlopuh requested a review from AntonLydike as a code owner July 15, 2023 07:41

superlopuh self-assigned this Jul 15, 2023

superlopuh requested review from math-fehr, webmiche and georgebisbas as code owners July 15, 2023 07:41

remove irdl annotation on abstract base class

3a0097a

superlopuh mentioned this pull request Jul 15, 2023

dialect: Refactor riscv register #1269

Closed

webmiche changed the title ~~dialects: (xdsl) remove Register class~~ dialects: (riscv) remove Register class Jul 15, 2023

kingiler reviewed Jul 15, 2023

View reviewed changes

xdsl/dialects/riscv.py Show resolved Hide resolved

kingiler reviewed Jul 15, 2023

View reviewed changes

compor approved these changes Jul 17, 2023

View reviewed changes

xdsl/dialects/riscv.py Outdated Show resolved Hide resolved

adutilleul approved these changes Jul 17, 2023

View reviewed changes

AntonLydike approved these changes Jul 17, 2023

View reviewed changes

xdsl/dialects/riscv.py Outdated Show resolved Hide resolved

superlopuh added 3 commits July 17, 2023 11:06

RegisterAttr -> IntRegisterAttr

3ce3e73

rename back to type

d5192c2

no such thing as a class property

d370965

kingiler requested changes Jul 17, 2023

View reviewed changes

xdsl/dialects/riscv.py Outdated Show resolved Hide resolved

superlopuh added 2 commits July 17, 2023 11:14

raise NotImplementedError

f5480c8

Merge branch 'main' into sasha/wm/register-attr

b2750a3

superlopuh requested a review from kingiler July 17, 2023 10:15

kingiler requested changes Jul 17, 2023

View reviewed changes

superlopuh requested a review from kingiler July 17, 2023 10:21

Merge branch 'main' into sasha/wm/register-attr

bb6cace

kingiler requested changes Jul 17, 2023

View reviewed changes

xdsl/dialects/riscv.py Outdated Show resolved Hide resolved

xdsl/dialects/riscv.py Show resolved Hide resolved

superlopuh requested a review from kingiler July 17, 2023 14:24

tobiasgrosser approved these changes Jul 17, 2023

View reviewed changes

superlopuh added 2 commits July 18, 2023 07:29

()

dcf2ff6

Merge branch 'main' into sasha/wm/register-attr

b53503b

kingiler approved these changes Jul 18, 2023

View reviewed changes

superlopuh merged commit b9fb446 into main Jul 18, 2023
10 checks passed

superlopuh deleted the sasha/wm/register-attr branch July 18, 2023 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (riscv) remove Register class #1281

dialects: (riscv) remove Register class #1281

superlopuh commented Jul 15, 2023 •

edited

codecov bot commented Jul 15, 2023 •

edited

kingiler Jul 15, 2023

superlopuh Jul 15, 2023

superlopuh Jul 15, 2023

superlopuh Jul 15, 2023

kingiler Jul 17, 2023

superlopuh Jul 17, 2023

tobiasgrosser Jul 17, 2023

kingiler Jul 15, 2023

kingiler Jul 15, 2023

compor left a comment •

edited

AntonLydike left a comment

kingiler left a comment

kingiler left a comment

tobiasgrosser commented Jul 17, 2023

tobiasgrosser left a comment

superlopuh commented Jul 18, 2023 •

edited

kingiler commented Jul 18, 2023

tobiasgrosser commented Jul 18, 2023

tobiasgrosser commented Jul 18, 2023

dialects: (riscv) remove Register class #1281

dialects: (riscv) remove Register class #1281

Conversation

superlopuh commented Jul 15, 2023 • edited

codecov bot commented Jul 15, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor left a comment • edited

Choose a reason for hiding this comment

AntonLydike left a comment

Choose a reason for hiding this comment

kingiler left a comment

Choose a reason for hiding this comment

kingiler left a comment

Choose a reason for hiding this comment

tobiasgrosser commented Jul 17, 2023

tobiasgrosser left a comment

Choose a reason for hiding this comment

superlopuh commented Jul 18, 2023 • edited

kingiler commented Jul 18, 2023

tobiasgrosser commented Jul 18, 2023

tobiasgrosser commented Jul 18, 2023

superlopuh commented Jul 15, 2023 •

edited

codecov bot commented Jul 15, 2023 •

edited

compor left a comment •

edited

superlopuh commented Jul 18, 2023 •

edited