dialects: riscv-func add initial function call lowering #929

eymay · 2023-05-08T16:28:11Z

together with prologue and epilogue register constraints compliant to conventions.

Integrated this branch with reference to the sasha/workshop-all branch.

superlopuh

Thank you! There are a few changes from my branch that I was hoping to clean up before merging, could you please address the comments?

xdsl/dialects/riscv_func.py

xdsl/transforms/lower_riscv_func.py

xdsl/dialects/riscv.py

superlopuh

Thank you, code mostly looks good to me, could you please remove the remaining TODOs?

xdsl/xdsl_opt_main.py

xdsl/transforms/lower_riscv_func.py

xdsl/dialects/riscv_func.py

superlopuh · 2023-05-17T10:33:26Z

GitHub automatically deleted the target branch of this PR, and clsoed the PR instead of retargeting it to main. Sorry about that.

superlopuh

Could you please rebase again?

xdsl/dialects/riscv_func.py

xdsl/dialects/riscv.py

xdsl/dialects/riscv_func.py

xdsl/transforms/lower_riscv_func.py

xdsl/dialects/riscv_func.py

…gue register constraints compliant to conventions

… from JalOp def to Func CallOp

nazavode

Looks really nice, thanks! Left just one question in review.

nazavode · 2023-05-19T14:13:19Z

xdsl/transforms/lower_riscv_func.py

+        if func_op is not None:
+            assert isinstance(func_op, riscv_func.FuncOp)
+            if func_op.func_name.data == "main":
+                # Main does not return, call exit syscall instead


Probably I'm missing something here but why main doesn't return?

Where would it return to? My understanding is that main is kind of just the entry point to the .text section, and not a "real" function

main always returns as a regular function, what happens later depends on the platform. On bare metal it's usually a super simple wrapper (crt0) that takes care of the return value but since we already have syscalls in the mix we are assuming a full fledged OS (e.g.: on linux-gnu is _libc_start_main that also does some flavor of exit syscall). I would say that the compiler has zero knowledge of who's going to invoke main (not really sure it's always true tho...).

...it's a much bigger topic tho, maybe this PR is not the right place to figure it out :) Approving as it is because it's really nice overall.

Ah no I'd love your input, we don't need to merge it as-is. It seems that we could simplify this PR by removing the syscall special case, if I understand you correctly. At the least, some lever higher than riscv-func could be made to deal with this, maybe the llvm to riscv lowering.

I think the truth is somewhere in the middle and that seems to be acknowledged in the C standard at least:
A return statement with a integer compatible value implies a call to exit.
If there is a plain return the termination status to the host environment is undefined which is the same as declaring void main().

More frustrating details here: https://en.cppreference.com/w/c/language/main_function.

That seems to be at a slightly higher level of abstraction to me, I think that's something that should be handled at the llvm level, unless I'm missing something

No objection there.

Thank you for the discussion, removed special handling of main function

Thanks for the changes, looks super neat!

superlopuh · 2023-05-19T17:13:33Z

LGTM, @AntonLydike, WDYT?

AntonLydike

It's very nice to see work continue in this direction! Although I want to re-iterate, that I feel that we are "just winging" API/ABI here. I don't think that's a very good direction to take.

Furthermore, I think we need some unit tests on the verifiers here!

AntonLydike · 2023-05-22T14:29:11Z

xdsl/dialects/riscv_func.py

+    name = "riscv_func.call"
+    args: Annotated[VarOperand, riscv.RegisterType]
+    func_name: OpAttr[StringAttr]
+    result: Annotated[OptOpResult, riscv.RegisterType]


I'm pretty sure that we want to have VarResult here instead. It's common to return values in multiple registers to prevent values spilling to the stack (e.g. when returning a 64 bit value, or a struct)

AntonLydike · 2023-05-22T14:30:07Z

xdsl/dialects/riscv_func.py

+    """RISC-V function return operation"""
+
+    name = "riscv_func.return"
+    value: Annotated[OptOperand, riscv.RegisterType]


Should also be VarOperand

AntonLydike · 2023-05-22T14:31:06Z

xdsl/dialects/riscv_func.py

+    """RISC-V function definition operation"""
+
+    name = "riscv_func.func"
+    args: Annotated[VarOperand, riscv.RegisterType]


Why does the function definition take arguments here?

Ah that's a good point, they should not be here indeed

AntonLydike · 2023-05-22T14:31:33Z

xdsl/dialects/riscv_func.py

+    args: Annotated[VarOperand, riscv.RegisterType]
+    func_name: OpAttr[StringAttr]
+    func_body: SingleBlockRegion
+


I feel this is missing a function_type style annotation? @superlopuh what's your opinion on that?

I'm still not exactly sure when we need it and when we don't. I guess it doesn't hurt, and should help with printing/parsing more cleanly

xdsl/dialects/riscv_func.py

xdsl/transforms/lower_riscv_func.py

JosseVanDelm

Thank you @eymay ! This looks really cool!
But maybe we should make issues for the cases that are not covered yet:

(register spilling)
non-main functions

xdsl/dialects/riscv_func.py

xdsl/transforms/lower_riscv_func.py

superlopuh · 2023-05-31T15:33:43Z

Would be nice to have this soon, I'm happy to address remaining comments and merge in the next couple of days

superlopuh · 2023-05-31T22:26:27Z

@AntonLydike I addressed your comments

@compor

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 #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. --------- Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>

AntonLydike

LGTM

JosseVanDelm · 2023-06-01T09:29:04Z

xdsl/dialects/riscv_func.py

@@ -58,18 +58,18 @@ class CallOp(IRDLOperation):
    name = "riscv_func.call"
    args: Annotated[VarOperand, riscv.RegisterType]
    func_name: OpAttr[StringAttr]
-    result: Annotated[OptOpResult, riscv.RegisterType]
+    ress: Annotated[VarOpResult, riscv.RegisterType]


nit: should this be results or res? ress looks a bit weird?

results is reserved, res to me implies singular, and ress is weird, alternative suggestions welcome

To be honest results already works, I expect people will use that, we can always rename it later

superlopuh requested changes May 8, 2023

View reviewed changes

superlopuh assigned eymay May 8, 2023

georgebisbas added the dialects Changes on the dialects label May 10, 2023

superlopuh force-pushed the sasha/riscv/riscv-ssa-syscall branch 2 times, most recently from 07524c3 to 876f15a Compare May 13, 2023 10:45

eymay force-pushed the sasha/riscv/riscv-ssa-syscall branch from 26f3cc4 to 51b2390 Compare May 15, 2023 16:30

superlopuh requested changes May 15, 2023

View reviewed changes

xdsl/xdsl_opt_main.py Outdated Show resolved Hide resolved

xdsl/transforms/lower_riscv_func.py Outdated Show resolved Hide resolved

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

superlopuh deleted the branch xdslproject:main May 17, 2023 10:07

superlopuh closed this May 17, 2023

superlopuh reopened this May 17, 2023

superlopuh changed the base branch from sasha/riscv/riscv-ssa-syscall to main May 17, 2023 10:35

eymay force-pushed the sasha/riscv/riscv-ssa-syscall branch from 2ae064f to 7185a6f Compare May 17, 2023 19:42

superlopuh requested changes May 17, 2023

View reviewed changes

eymay force-pushed the sasha/riscv/riscv-ssa-syscall branch from 7185a6f to b8ba98e Compare May 18, 2023 13:56

superlopuh and others added 15 commits May 18, 2023 18:15

dialects: add riscv_ssa with lowering from syscall op

5d17ba9

remove SCALL_EXIT

40d4c41

rename riscv_ssa to riscv_structured and add operand to test

fb35769

riscv_structured -> riscv_func

b5d5a32

fix ir

7213ce7

structured to func

bbaa478

dialects: add function call lowering together with prologue and epilo…

d38280b

…gue register constraints compliant to conventions

Removed comments and str related constructs

6b34508

Rebased changes and fixed rename issues

d5972a2

rename and bound check fix

60f5fd8

Revert deletion of Syscall sections and rename fix

261f8a6

Function call returns RA register by default, this behaviour is moved…

bb070cf

… from JalOp def to Func CallOp

Added args to riscv func for verification

ed07303

Rename and todo cleanup

1728633

Remove SectionOp

82544ab

superlopuh requested review from compor and AntonLydike May 19, 2023 09:53

nazavode reviewed May 19, 2023

View reviewed changes

nazavode approved these changes May 19, 2023

View reviewed changes

eymay added 3 commits May 19, 2023 19:32

Removed return handling specific to main

662755a

basic docstrings for riscv_func operations

37c696d

test updated due to removal of special handling of main

c402214

AntonLydike requested changes May 22, 2023

View reviewed changes

JosseVanDelm approved these changes May 22, 2023

View reviewed changes

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

xdsl/transforms/lower_riscv_func.py Outdated Show resolved Hide resolved

adutilleul mentioned this pull request May 24, 2023

dialects: riscv: Add basic register allocator strategy #995

Merged

Merge branch 'main' into sasha/riscv/riscv-ssa-syscall

dd3ea13

superlopuh requested review from math-fehr, webmiche and georgebisbas as code owners May 31, 2023 21:50

superlopuh added 6 commits May 31, 2023 23:11

variadic function results

1324367

remove args on FuncOp

3f96c69

remove outdated comment

a69558c

remove SCALL_EXIT constant

86947fa

greedy pattern rewrite walker

8db03bc

dont edc

1600604

superlopuh requested a review from AntonLydike May 31, 2023 22:26

superlopuh added 2 commits June 1, 2023 10:10

Merge branch 'main' into sasha/riscv/riscv-ssa-syscall

3000ee3

use HasParent and remove verify on riscv.func

bed95f7

AntonLydike approved these changes Jun 1, 2023

View reviewed changes

JosseVanDelm reviewed Jun 1, 2023

View reviewed changes

superlopuh merged commit 4343a8f into xdslproject:main Jun 1, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: riscv-func add initial function call lowering #929

dialects: riscv-func add initial function call lowering #929

eymay commented May 8, 2023

superlopuh left a comment

superlopuh left a comment

superlopuh commented May 17, 2023

superlopuh left a comment

nazavode left a comment

nazavode May 19, 2023

superlopuh May 19, 2023

nazavode May 19, 2023

nazavode May 19, 2023

superlopuh May 19, 2023

compor May 19, 2023 •

edited

superlopuh May 19, 2023

compor May 19, 2023

eymay May 19, 2023

nazavode May 19, 2023

superlopuh commented May 19, 2023

AntonLydike left a comment

AntonLydike May 22, 2023

AntonLydike May 22, 2023

AntonLydike May 22, 2023

superlopuh May 22, 2023

AntonLydike May 22, 2023

superlopuh May 22, 2023

JosseVanDelm left a comment

superlopuh commented May 31, 2023

superlopuh commented May 31, 2023

AntonLydike left a comment

JosseVanDelm Jun 1, 2023

superlopuh Jun 1, 2023

superlopuh Jun 1, 2023

JosseVanDelm Jun 1, 2023

dialects: riscv-func add initial function call lowering #929

dialects: riscv-func add initial function call lowering #929

Conversation

eymay commented May 8, 2023

superlopuh left a comment

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

superlopuh commented May 17, 2023

superlopuh left a comment

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

Choose a reason for hiding this comment

compor May 19, 2023 • edited

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 commented May 19, 2023

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

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

JosseVanDelm left a comment

Choose a reason for hiding this comment

superlopuh commented May 31, 2023

superlopuh commented May 31, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor May 19, 2023 •

edited