dialects: riscv: Add basic register allocator strategy #995

adutilleul · 2023-05-24T10:27:01Z

The current register allocator for the RISC-V dialect exclusively utilizes j-registers for unallocated values. However, @compor and I propose an improvement over the existing allocator by introducing a naive strategy that leverages the available real RISC-V registers for each block. In case the real registers are exhausted, the allocator gracefully falls back to j-registers.

This enhancement brings forth several important questions that need to be addressed at this stage:

Determining the Set of Available Registers: The choice of available registers depends on the ABI and the adopted calling convention. So, we probably want to determine a target for now and see how it involves other topics (like function handling see dialects: riscv-func add initial function call lowering #929).
Efficiency Requirements for Future Work: We might need to establish the desired level of efficiency for the register allocator (linear scan, ...) and the relevant use cases.

Moreover, this improvement is contingent on the merging of the LabelOp into the main branch. For further details regarding the LabelOp merge, please refer to the following pull request #994.

codecov · 2023-05-24T10:29:34Z

Codecov Report

Patch coverage: 97.22% and project coverage change: +0.10 🎉

Comparison is base (4530299) 86.98% compared to head (b3fff0a) 87.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #995      +/-   ##
==========================================
+ Coverage   86.98%   87.09%   +0.10%     
==========================================
  Files         126      127       +1     
  Lines       19438    19544     +106     
  Branches     2951     2960       +9     
==========================================
+ Hits        16909    17022     +113     
+ Misses       2029     2021       -8     
- Partials      500      501       +1

Impacted Files	Coverage Δ
xdsl/transforms/riscv_register_allocation.py	`94.44% <91.17%> (-5.56%)`	⬇️
docs/Toy/toy/tests/test_regalloc.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

tobiasgrosser · 2023-05-24T10:40:23Z

Very cool. Would it make sense to have some filecheck test cases?

compor · 2023-05-24T10:50:26Z

Very cool. Would it make sense to have some filecheck test cases?

Eventually yes, but IMHO it will be something very brittle at this point due to the way we "hand out" available registers.
So, not really sure.

tobiasgrosser · 2023-05-24T10:56:21Z

Very cool. Would it make sense to have some filecheck test cases?

Eventually yes, but IMHO it will be something very brittle at this point due to the way we "hand out" available registers. So, not really sure.

That's the point. If it is brittle we want to see this exposed. In particular, the registers assigned should at least be deterministic.

AntonLydike

Overall a step in a very good direction! Just some minor things I'd like to see cleaned up. If you need help feel free to drop by my office!

xdsl/transforms/riscv_register_allocation.py

AntonLydike · 2023-05-24T17:28:33Z

xdsl/transforms/riscv_register_allocation.py

    """

    name = "riscv-allocate-registers"

+    allocation_type: Union[str, None] = None


I wouls suggest something like:

allocation_type: str = 'global-jregs'

Or whatever the magic constant is.

AntonLydike · 2023-05-24T17:30:09Z

xdsl/transforms/riscv_register_allocation.py

+    GlobalJRegs = 0
+    BlockNaive = 1


Shouldn't these have other values? I'm a bit frustated with typing enums in python, maybe a dictionary is better suited here? In a dict you can at least map names to one specific type. Python doesn't let you specify a type of the enum values...

What do you mean by other values? Is this a tooling deficiency in typing or a general thing?
I cannot find a lot of evidence supporting this opinion online.
I thought the point of an enum was to represent a custom type.
Would using IntEnum be better?

Moreover, I see that it is used elsewhere in our codebase.

xdsl/riscv_asm_writer.py

nazavode

Beautiful, thanks. No additional comments since I would have added the same remarks @AntonLydike pointed out already (especially having #994 merged first).

superlopuh · 2023-05-25T12:56:24Z

requirements-optional.txt

@@ -8,6 +8,7 @@ nbval<0.11
 filecheck<0.0.24
 lit<17.0.0
 pre-commit==3.3.2
+git+https://github.com/antonlydike/riscemu.git@25d059da090760862f9143478524ed6daf0ab449#egg=riscemu


We can't actually include this yet, or at least would need to do this in a separate PR where we can discuss the extra dependency separately. I don't think the register allocation requires this change, even if it's useful for debugging, so I'd recommend taking out all riscemu related changes for now.

superlopuh · 2023-05-25T12:57:06Z

xdsl/transforms/riscv_register_allocation.py

 from xdsl.dialects.builtin import ModuleOp
 from xdsl.dialects.riscv import Register, RegisterType, RISCVOp
 from xdsl.ir import MLContext
 from xdsl.passes import ModulePass


-class RegisterAllocator:
+class RegisterAllocatorStrategy:


Suggested change

class RegisterAllocatorStrategy:

class AbstractRegisterAllocator(ABC):

superlopuh · 2023-05-25T12:59:58Z

xdsl/transforms/riscv_register_allocation.py

    def apply(self, ctx: MLContext, op: ModuleOp) -> None:
-        allocator = RegisterAllocator()
+        allocator_strategies = {


superlopuh · 2023-05-25T13:00:26Z

xdsl/transforms/riscv_register_allocation.py

    """

    name = "riscv-allocate-registers"

+    allocation_type: str = "GlobalJRegs"


Suggested change

allocation_type: str = "GlobalJRegs"

allocation_strategy: str = "GlobalJRegs"

superlopuh

Thank you! Would be good to iterate on the changes a bit, I left a few comments

…tilleul/riscv/regalloc

superlopuh · 2023-05-29T09:03:31Z

xdsl/transforms/riscv_register_allocation.py

@@ -57,16 +62,16 @@ def allocate_registers(self, module: ModuleOp) -> None:
                        assert isinstance(result.typ, RegisterType)
                        if result.typ.data.name is None:
                            # If we run out of real registers, allocate a j register
-                            if block_registers == list():
+                            if len(block_registers) == 0:


Suggested change

if len(block_registers) == 0:

if not block_registers:

superlopuh · 2023-05-29T09:06:16Z

xdsl/transforms/riscv_register_allocation.py

+        let's just assume that we have all the registers available for our use except the one explicitly reserved by the default riscv ABI.
+        """
+
+        self.available_registers = OrderedDict(Register.ABI_INDEX_BY_NAME)


Looking at it more in detail, it seems like I gave a bad recommendation here. Only reserved_registers should be a set, the others were better the way you first wrote them, as lists.

superlopuh

thank you!

AntonLydike

Looks a lot better already, I think you still have some label PR changes in here. I'd suggest either rebasing or merging main into this branch so it contains only your changes.

AntonLydike · 2023-05-30T08:45:31Z

tests/dialects/test_riscv.py

+def test_label_op_without_comment():
+    label_str = "mylabel"
+    label_op = riscv.LabelOp(label_str)
+
+    assert label_op.label.data == label_str
+
+    code = riscv_code(ModuleOp([label_op]))
+    assert code == f"{label_str}:\n"
+
+
+def test_label_op_with_comment():
+    label_str = "mylabel"
+    label_op = riscv.LabelOp(f"{label_str}", comment="my label")
+
+    assert label_op.label.data == label_str
+
+    code = riscv_code(ModuleOp([label_op]))
+    assert code == f"{label_str}:                                         # my label\n"
+
+
+def test_label_op_with_region():
+    @Builder.implicit_region
+    def label_region():
+        a1_reg = TestSSAValue(riscv.RegisterType(riscv.Registers.A1))
+        a2_reg = TestSSAValue(riscv.RegisterType(riscv.Registers.A2))
+        riscv.AddOp(a1_reg, a2_reg, rd=riscv.Registers.A0)
+
+    label_str = "mylabel"
+    label_op = riscv.LabelOp(f"{label_str}", region=label_region)
+
+    assert label_op.label.data == label_str
+
+    code = riscv_code(ModuleOp([label_op]))
+    assert code == f"{label_str}:\n    add a0, a1, a2\n"
+
+


There are still changes from the label op in this PR, or am I mistaken?

yeah, I'd suggest rebasing on the main branch since that PR is now merged.

compor and others added 7 commits May 24, 2023 08:48

dialects/riscv: Add LabelOp to RISCV dialect

d13dc82

dialects/riscv: Add optional regions to LabelOp

18bb1bd

dialects: risc: Add assembly writer case for LabelOp

e202937

dialects: risc: Add test case for LabelOp assembly writer

3a956a0

dialects: risc: Add test case for LabelOp with region

8125bd5

dialects: risc: Split test cases for LabelOp

e57fdc0

dialects: riscv: Add a naive register allocator strategy

38dd1ef

adutilleul requested review from math-fehr, webmiche, superlopuh, compor, AntonLydike, nazavode and georgebisbas as code owners May 24, 2023 10:27

dialects: riscv: fix some formatting for regalloc

37f37c0

adutilleul added the dialects Changes on the dialects label May 24, 2023

dialects: riscv: add some basic filecheck for regalloc

986653c

AntonLydike requested changes May 24, 2023

View reviewed changes

compor and others added 2 commits May 24, 2023 19:12

dialects/riscv: remove ctor

f6d7f7b

dialects: riscv: simplify RISCVRegisterAllocation

16c236e

nazavode reviewed May 25, 2023

View reviewed changes

nazavode mentioned this pull request May 25, 2023

dialects: Add the basic concept of compilation targets #980

Merged

superlopuh assigned adutilleul May 25, 2023

superlopuh changed the base branch from main to christos/riscv/labelop May 25, 2023 12:54

superlopuh reviewed May 25, 2023

View reviewed changes

superlopuh requested changes May 25, 2023

View reviewed changes

compor added 10 commits May 25, 2023 15:25

dialects/riscv: Add LabelOp to RISCV dialect

109290f

dialects/riscv: Add optional regions to LabelOp

a3366cf

dialects: risc: Add assembly writer case for LabelOp

ca0f2ce

dialects: risc: Add test case for LabelOp assembly writer

cd71742

dialects: risc: Add test case for LabelOp with region

0550202

dialects: risc: Split test cases for LabelOp

061da8e

dialects/riscv: Add docstring to LabelOp

8f6aa4a

dialects/riscv: Add filecheck tests

4a6283b

dialects/riscv: Remove trivial f-strings

39e0e4a

dialects/riscv: Use utility method for comment generation

fa25a79

compor force-pushed the christos/riscv/labelop branch from 169e3c2 to fa25a79 Compare May 25, 2023 14:25

adutilleul added 3 commits May 25, 2023 15:48

Merge remote-tracking branch 'origin/christos/riscv/labelop' into adu…

7b0ca72

…tilleul/riscv/regalloc

dialects: riscv: misc nitpicks in regalloc

06c477b

dialects: riscv: remove riscv-emu dep for regalloc tests

bd842f4

superlopuh requested changes May 29, 2023

View reviewed changes

dialects: riscv: stick to list for available_registers in regalloc

a7ec22a

compor force-pushed the christos/riscv/labelop branch from 7cf4648 to 053876a Compare May 29, 2023 09:49

superlopuh approved these changes May 29, 2023

View reviewed changes

Base automatically changed from christos/riscv/labelop to main May 29, 2023 12:11

AntonLydike approved these changes May 30, 2023

View reviewed changes

adutilleul added 2 commits May 30, 2023 11:04

Merge branch 'main' into adutilleul/riscv/regalloc

6777ce5

dialects: riscv: fix formatting in test_regalloc.py

bd3e7a1

nazavode approved these changes May 31, 2023

View reviewed changes

adutilleul added 2 commits June 1, 2023 09:31

Merge branch 'main' into adutilleul/riscv/regalloc

c72eb63

dialects: riscv: fix type issue in regalloc filecheck test

b3fff0a

adutilleul merged commit e1e8e2d into main Jun 1, 2023
12 checks passed

adutilleul deleted the adutilleul/riscv/regalloc branch June 1, 2023 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: riscv: Add basic register allocator strategy #995

dialects: riscv: Add basic register allocator strategy #995

adutilleul commented May 24, 2023

codecov bot commented May 24, 2023 •

edited

tobiasgrosser commented May 24, 2023

compor commented May 24, 2023

tobiasgrosser commented May 24, 2023 •

edited

AntonLydike left a comment

AntonLydike May 24, 2023

AntonLydike May 24, 2023

compor May 24, 2023 •

edited

nazavode left a comment

superlopuh May 25, 2023

superlopuh May 25, 2023

superlopuh May 25, 2023

superlopuh May 25, 2023

superlopuh left a comment

superlopuh May 29, 2023

superlopuh May 29, 2023

superlopuh May 29, 2023

superlopuh left a comment

AntonLydike left a comment

AntonLydike May 30, 2023

compor May 30, 2023

	class RegisterAllocatorStrategy:
	class AbstractRegisterAllocator(ABC):

	allocation_type: str = "GlobalJRegs"
	allocation_strategy: str = "GlobalJRegs"

dialects: riscv: Add basic register allocator strategy #995

dialects: riscv: Add basic register allocator strategy #995

Conversation

adutilleul commented May 24, 2023

codecov bot commented May 24, 2023 • edited

Codecov Report

tobiasgrosser commented May 24, 2023

compor commented May 24, 2023

tobiasgrosser commented May 24, 2023 • edited

AntonLydike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor May 24, 2023 • edited

Choose a reason for hiding this comment

nazavode left a 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

superlopuh left a 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

superlopuh left a comment

Choose a reason for hiding this comment

AntonLydike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 24, 2023 •

edited

tobiasgrosser commented May 24, 2023 •

edited

compor May 24, 2023 •

edited