[compiler-rt][RISC-V] ILP32E/LP64E Save/Restore Grouping #95398
Conversation
This changes the save/restore procedures to save/restore registers one by one - to 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). This is a potential break for all of the ILP32E/LP64E ABI - we may instead have to teach the compiler to emit the CFI information correctly for the grouping we already have implemented (because that grouping matches GCC). It depends on how intentional we think the grouping is in the original ILP32E/LP64E save/restore implementation was, and whether we think we can fix that now.
|
gcc-mirror/gcc@09baee1 is the original GCC commit that introduced this ABI. I don't see any justification for this, but haven't trawled mailing lists. Given part of the point of RVE is to save memory, it's odd to always burn the stack space for the sake of a few extra instructions in the libcalls, but it's probably also true that you often need the saved registers. I think we need to form cross-toolchain consensus, write it down in the toolchain conventions document and go with that, whichever it is. CC @kito-cheng as co-author of the patch in question. |
|
@kito-cheng ping, could we have your input on this and/or #95390 |
|
That's two different flavor of implementation, one for smaller code size (but few byte smaller only) , and another one is smaller stack usage, I have to say I like the later one which is this PR :P Honestly I don't have too much memory about why we chose that approach, my best guess it could reuse same CFI logic, however...ironically I found the CFI directive is wrong on the GCC side for save restore routines for ilp32e...so I think it's good timing to document that down on toolchain conventions and then I gonna fix that on GCC side. |
|
I'm glad you agree about stack usage :) I'll try to draft something in the toolchain conventions this week or on Monday, so we can start working through pinning down what the right CFI information is for compilers to emit for these routines. |
RIP my intentions, but I finally wrote something: riscv-non-isa/riscv-toolchain-conventions#70 |
|
@tclin914 Do you have a setup for testing ilp32e executables? I would appreciate you running your tests on this patch if you could. I think it's the right approach, but I'm not sure and I don't have an ilp32e setup to hand. |
Quoted from my previous comments in https://reviews.llvm.org/D70401:
Sorry I can't remember the details and how to set up the environment (you will need a ELF toolchain IIRC). I hope this can be helpful. |
This changes the save/restore procedures to save/restore registers one by one - to 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).
This is a potential break for all of the ILP32E/LP64E ABI - we may instead have to teach the compiler to emit the CFI information correctly for the grouping we already have implemented (because that grouping matches GCC). It depends on how intentional we think the grouping is in the original ILP32E/LP64E save/restore implementation was, and whether we think we can fix that now.