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

Risc-v: proper management of the return address #939

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

eponier
Copy link
Contributor

@eponier eponier commented Oct 23, 2024

On RISC-V, the return address is read from register RA. This was not reflected in the model and generated wrong programs.

The RAstack case of return_address_location is given an additional optional argument ra_return that specifies what register to use (if any) when returning from a function. linearization is adapted to generate the right code. reg alloc and merge_varmaps take into account this potential new register.

I'm unsure of the changes I did. In particular, for linearization_proof, I have the impression that there is some kind of duplication between the semantics (ra_vm, ra_undef, ...) and the proof that introduces killed_by_entry, killed_on_exit, killed_by_exit. I managed to have a working version of the proof, but this may not be the principled one.

And the history is horrible, I still have some changes to do, I'll clean it at some point.

@eponier
Copy link
Contributor Author

eponier commented Oct 23, 2024

I also have to rebase on branch risc-v, and to undo the commit adding ra to the callee_saved registers (even if that could make sense, ra contains the return address both when entering and leaving the function).

@eponier
Copy link
Contributor Author

eponier commented Oct 24, 2024

Btw, there is something to be checked about RISC-V, related to the linearization_params. The code was copied from arm, and it should work. But if I'm not mistaken, there are two constraints on ARM that make the code more complex:

  1. there are some operations that we cannot do directly on RSP
  2. instructions do not accept large immediate.

For RISC-V, 2. is true, but 1. is not. Does this mean we should adapt a bit the code?

@eponier eponier marked this pull request as ready for review October 24, 2024 11:59
@eponier
Copy link
Contributor Author

eponier commented Nov 5, 2024

I've just rebased on top of #934

@eponier
Copy link
Contributor Author

eponier commented Nov 5, 2024

Btw, there is something to be checked about RISC-V, related to the linearization_params. The code was copied from arm, and it should work. But if I'm not mistaken, there are two constraints on ARM that make the code more complex:

1. there are some operations that we cannot do directly on RSP

2. instructions do not accept large immediate.

For RISC-V, 2. is true, but 1. is not. Does this mean we should adapt a bit the code?

I created issue #954 to keep track of that part.

On RISC-V, the return address is read from register RA. This was not
reflected in the model and generated wrong programs.

The RAstack case of return_address_location is given an additional
optional argument ra_return that specifies what register to use (if any)
when returning from a function. linearization is adapted to generate the
right code. reg alloc and merge_varmaps take into account this potential
new register.
@eponier
Copy link
Contributor Author

eponier commented Nov 5, 2024

I also have to rebase on branch risc-v, and to undo the commit adding ra to the callee_saved registers (even if that could make sense, ra contains the return address both when entering and leaving the function).

done

@bgregoir bgregoir merged commit d03e378 into risc-v Nov 5, 2024
1 check passed
@bgregoir bgregoir deleted the risc-v-return-address branch November 5, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants