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

[compiler-rt][RISC-V] Save/Restore for E goes with ABI #95390

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

lenary
Copy link
Member

@lenary lenary commented Jun 13, 2024

When compiling for the ILP32E/LP64E ABIs, even on a RISC-V machine with
i, we should be using the ILP32E/LP64E save/restore routines, so use
the right preprocessor macro.


Previous Description Below:

This makes two significant changes, which I believe are both bugfixes.

  • When compiling for the ILP32E/LP64E ABIs, even on a RISC-V machine with i, we should be using the ILP32E/LP64E save/restore routines, so use the right preprocessor macro.
  • Saves/restores registers in batches that match the stack alignment for the ILP32E/LP64E ABIs, rather than the larger batches of the conventional ABIs. The implementations of the save routines are not tail-shared, to reduce the number of instructions. I think this also helps code size but I need to check this again.

I would expect (but haven't measured) that the majority of functions compiled for the ILP32E/LP64E ABIs will in fact use both callee-saved registers, and therefore there are still savings to be had, but I think those can come later, with more data (especially if those changes are just to the instruction sequences we use to save the registers, rather than the number and alignment of how this is done).

From what I can tell, this matches the CFI information that both clang and GCC emit: https://godbolt.org/z/ozY3z8Yx5

@lenary
Copy link
Member Author

lenary commented Jun 13, 2024

This is a follow up to fix code added in #88252. Adding those reviewers too.

@lenary lenary requested review from kito-cheng and wangpc-pp June 13, 2024 11:05
Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

libgcc is as our compiler-rt is today. Switching to look at the ABI not the ISA makes sense to me, and sounds like an oversight in GCC that got mirrored to LLVM (after all, it's an unusual combination to use), but we need to match GCC's ABI for RVE save/restore libcalls, which is to always save all three registers.

When compiling for the ILP32E/LP64E ABIs, even on a RISC-V machine with
`i`, we should be using the ILP32E/LP64E save/restore routines, so use
the right preprocessor macro.
@lenary lenary force-pushed the pr/save-restore-ilp32e-lp64e branch from 62e79f6 to 4f8212c Compare June 13, 2024 11:34
@lenary
Copy link
Member Author

lenary commented Jun 13, 2024

I'll split this into two PRs, one for the preprocessor macro, one for the changes to grouping.

My feeling is that there is no specifically-intentional grouping for ilp32e/lp64e given these bugs (it was an oversight by the original implementers), but I realise that also doesn't mean we can retroactively change the grouping, and it may just be that we need to change the CFI we emit instead (and file the same two bugs against GCC). New PR for changing the grouping will be posted momentarily.

@lenary
Copy link
Member Author

lenary commented Jun 13, 2024

#95398 Is the other half of the Split, with the more controversial changes.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 13, 2024

(Though I believe the title is a bit outdated)

@rOptimizer
Copy link

Please notice, that gcc behaves differently with -march=rv32e and it then matches it's library implementation.
See https://godbolt.org/z/WdTnrWj8T

@lenary lenary changed the title [compiler-rt][RISC-V] Save-Restore for ILP32E/LP64E ABIs [compiler-rt][RISC-V] Save/Restore for E goes with ABI Jun 13, 2024
@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 13, 2024

Copy link
Member

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

LGTM, I believe it's right change, and I know there is some strange situations will use rv32i/rv64i with ilp32e/lp64e ABI...

@lenary lenary merged commit eb97739 into llvm:main Jul 11, 2024
6 checks passed
@lenary lenary deleted the pr/save-restore-ilp32e-lp64e branch July 11, 2024 12:06
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
When compiling for the ILP32E/LP64E ABIs, even on a RISC-V machine with
`i`, we should be using the ILP32E/LP64E save/restore routines, so use
the right preprocessor macro.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants