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

backend: (riscv) Add prologue epilogue insertion #2752

Merged
merged 9 commits into from
Jun 20, 2024

Conversation

zero9178
Copy link
Contributor

The RISC-V call ABI requires a few registers, if clobbered, to be restored by the callee prior to the function returning. This has so far not been implemented meaning that any C compiler calling an xDSL generated function may cause undefined behaviour.

This PR adds a pass performing the required register saving and restoring by pushing and popping them from the stack. The registers are saved in the prologue at the beginning of the function and an epilogue restoring them is emitted before every return operation.

The pass is required to run after register allocation to see the clobbering of callee-preserved registers. It itself does not require register allocation to be run afterward.

The RISC-V call ABI requires a few registers, if clobbered, to be restored by the callee prior to the function returning.
This has so far not been implemented meaning that any C compiler calling an xDSL generated function may cause undefined behaviour.

This PR adds a pass performing the required register saving and restoring by pushing and popping them from the stack. The registers are saved in the prologue at the beginning of the function and an epilogue restoring them is emitted before every return operation.

The pass is required to run after register allocation to see the clobbering of callee-preserved registers. It itself does not require register allocation to be run afterward.
@zero9178 zero9178 added the backend Compiler backend in xDSL label Jun 18, 2024
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.80%. Comparing base (79eeff0) to head (9a81b03).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2752   +/-   ##
=======================================
  Coverage   89.79%   89.80%           
=======================================
  Files         372      373    +1     
  Lines       47722    47777   +55     
  Branches     7310     7323   +13     
=======================================
+ Hits        42853    42904   +51     
- Misses       3732     3734    +2     
- Partials     1137     1139    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

Do MLIR passes guarantee idempotency? For some of the things I'd like to do, like automatic pipeline construction, idempotency is a very nice feature to have. I wonder if we could structure the stack handling in our backend somehow to guarantee that if we run a pass twice we get the same result. One obvious way might be to add an attribute to denote a function that's been processed already, that would be skipped if processed again, but maybe in a more integrated way with the register allocator? Happy to brainstorm.

@zero9178
Copy link
Contributor Author

Do MLIR passes guarantee idempotency?

They do not no.

One obvious way might be to add an attribute to denote a function that's been processed already, that would be skipped if processed again, but maybe in a more integrated way with the register allocator? Happy to brainstorm.

We can do that if you want. An attribute on the function makes sense to me if you want to track a phase in that sense. Do you want to see this already as part of this PR?

I am not sure how this pass requires any integration with the register allocator in any way. It only needs to see its output basically. This will change if we ever start spilling memory to stack as we'll probably want a meta-op representing a not-yet materialized stack allocation that this pass would materialize, but that is the only thing I can think of. That is future talk either way.

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

LGTM

@zero9178 zero9178 merged commit bc1b4b7 into main Jun 20, 2024
10 checks passed
@zero9178 zero9178 deleted the zero9178/riscv-prologue-epilogue-insertion branch June 20, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Compiler backend in xDSL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants