Skip to content
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

dialects: (riscv) add initial support for F extension #1188

Merged
merged 13 commits into from Jun 28, 2023

Conversation

adutilleul
Copy link
Collaborator

This PR introduces some changes needed to support the handling of floating point operations (F extension) in the riscv dialect and all the operations themselves, which is necessary for future works.

The main points are:

  • Split the class RegisterType into two different classes to distinguish between
    integer and floating point registers. This is handy when register allocating and
    also to ensure the soundness of the dialect in order to stay in line with the ISA.
    Thus, the user has to specify directly the type of the register when using it inside
    an operation (riscv.ireg or riscv.freg).
  • I'm still unsure if that's the best way of doing this, so I'm open to suggestions.
  • I've updated already most of the test suite, including the emulation tests, where I found some issues in the riscemu library which resulted in a PR (Fix some issues in the RV32F extension implementation AntonLydike/riscemu#27), still pending to be merged.

@adutilleul adutilleul added the dialects Changes on the dialects label Jun 23, 2023
@adutilleul adutilleul self-assigned this Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch coverage: 80.14% and project coverage change: -0.26 ⚠️

Comparison is base (bc047cc) 88.96% compared to head (0eef7d9) 88.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1188      +/-   ##
==========================================
- Coverage   88.96%   88.71%   -0.26%     
==========================================
  Files         166      166              
  Lines       22551    22855     +304     
  Branches     3404     3478      +74     
==========================================
+ Hits        20062    20275     +213     
- Misses       1945     2025      +80     
- Partials      544      555      +11     
Impacted Files Coverage Δ
xdsl/interpreters/riscv_emulator.py 86.48% <50.00%> (-7.46%) ⬇️
tests/dialects/test_riscv.py 92.46% <52.17%> (-7.54%) ⬇️
xdsl/backend/riscv/register_allocation.py 81.48% <73.07%> (-11.38%) ⬇️
xdsl/dialects/riscv.py 83.25% <82.50%> (-3.97%) ⬇️
tests/backend/riscv/test_regalloc.py 100.00% <100.00%> (ø)
tests/dialects/test_snitch.py 100.00% <100.00%> (ø)

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

@superlopuh
Copy link
Member

One thing to consider is to keep the integer register "Register" and add the floating point as "FloatingPointRegister", since integer registers is what we'll be using the most by far, and they're the only ones in the core spec

@adutilleul
Copy link
Collaborator Author

I have cleaned up the PR a bit to keep a smaller diff by applying the suggestion of @superlopuh.
I also removed the F extension emulation tests since I don't think the other PR will be merged soon, so I'll add them back in another PR when it's ready.

@adutilleul adutilleul marked this pull request as ready for review June 27, 2023 13:17
@adutilleul adutilleul changed the title [WIP] dialects: (riscv) add initial support for F extension dialects: (riscv) add initial support for F extension Jun 27, 2023
@@ -1396,7 +1513,7 @@ class BltuOp(RsRsOffOperation):


@irdl_op_definition
class BgeuOp(RsRsOffOperation):
class BgeuOp(RsRsOffOperationInteger):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This either appears are ...OperationInteger or ...IntegerOperation.
Is this on purpose? Shouldn't both be the latter?



@irdl_attr_definition
class FloatingRegisterType(Data[Register], TypeAttribute):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for being pedantic, but can you use Float instead of Floating?
I want to remember one mnemonic if possible :)

@@ -25,6 +25,7 @@ def module():
seven = riscv.LiOp(7).rd
forty_two = riscv.MulOp(six, seven).rd
riscv.CustomAssemblyInstructionOp("print", inputs=[forty_two], result_types=[])
riscv.ReturnOp()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was added in another PR, can you rebase/merge please?

@adutilleul adutilleul merged commit 00608a1 into main Jun 28, 2023
12 checks passed
@adutilleul adutilleul deleted the adutilleul/riscv/rv32f-float-support branch June 28, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants