[RISCV] Improper instructions were generated for global register variables sepcified by `-ffixed-reg` option.

[wan-yuqi]

Define a callee-saved RISCV register as a Global Register Variable. The prologue/epilogue instructions of the register will be generated, which will cause the value of the global register variable to be unexpected. GCC Global-Register-Variables

register void* x18 asm("x18"); // aka s2
void callee_saved() {
    x18 = (void*)0x10000;
}

compiling with option -ffixed-x18

callee_saved:
        addi    sp, sp, -16
        sw      s2, 12(sp)
        lui     s2, 16
        lw      s2, 12(sp)
        addi    sp, sp, 16
        ret

The value set for s2 register by lui s2 16 will be overwritten by the subsequenct lw s2 instruction.

compared with GCC：https://godbolt.org/z/WKKxT7sz8

Which I can find the earilest PR regarding the support of the -ffixed-reg option for RISCV is D67185. Perhaps it was inspired by ARM's support by -ffixed-reg(D56005).

However, after reviewing the modifications in this PR, I found that it only implements the behavior that the complier will not allocate the specified RISCV register when using the -ffixed-reg option, and may report errors in some scenariors. It appears to have ignored the prologue/epilogue instructions generated by compiler for callee-saved register used in function.

I dont know if anyone has already discovered this issue or has any solution.

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: nomore (wan-yuqi)

Define a callee-saved RISCV register as a Global Register Variable. The prologue/epilogue instructions of the register will be generated, which will cause the value of the global register variable to be unexpected. [GCC Global-Register-Variables](https://gcc.gnu.org/onlinedocs/gcc/Global-Register-Variables.html#Global-Register-Variables) ```C register void* x18 asm("x18"); // aka s2 void callee_saved() { x18 = (void*)0x10000; } ``` compiling with option `-ffixed-x18` ``` callee_saved: addi sp, sp, -16 sw s2, 12(sp) lui s2, 16 lw s2, 12(sp) addi sp, sp, 16 ret ``` The value set for `s2` register by `lui s2 16` will be overwritten by the subsequenct `lw s2` instruction. compared with GCC：https://godbolt.org/z/WKKxT7sz8 Which I can find the earilest PR regarding the support of the `-ffixed-reg` option for RISCV is [D67185](https://reviews.llvm.org/D67185). Perhaps it was inspired by ARM's support by `-ffixed-reg`([D56005](https://reviews.llvm.org/D56005)). However, after reviewing the modifications in this PR, I found that it only implements the behavior that the complier will not allocate the specified RISCV register when using the `-ffixed-reg` option, and may report errors in some scenariors. It appears to have ignored the prologue/epilogue instructions generated by compiler for callee-saved register used in function. I dont know if anyone has already discovered this issue or has any solution.

[svs-quic]

One thing we can maybe do is to remove the reserved registers from the list of callee-saved registers before assigning callee saved spill slots:

--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -1403,6 +1403,11 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
   if (CSI.empty())
     return true;

+  for (unsigned i = 0, e = CSI.size(); i < e; ++i) {
+    if (STI.isRegisterReservedByUser(CSI[i].getReg()))
+      CSI.erase(CSI.begin() + i);
+  }
+
   auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();

   if (RVFI->isPushable(MF)) {

This seems to work on the test case given above, but I am not really sure if it can cause any other issues.

[topperc]

One thing we can maybe do is to remove the reserved registers from the list of callee-saved registers before assigning callee saved spill slots:

--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -1403,6 +1403,11 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
   if (CSI.empty())
     return true;

+  for (unsigned i = 0, e = CSI.size(); i < e; ++i) {
+    if (STI.isRegisterReservedByUser(CSI[i].getReg()))
+      CSI.erase(CSI.begin() + i);
+  }
+
   auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();

   if (RVFI->isPushable(MF)) {

This seems to work on the test case given above, but I am not really sure if it can cause any other issues.

It wouldn't be compatible with -msave-restore or the Zcmp push/pop instructions.

[lenary]

Yeah, we need to be really careful with this and -msave-restore, as it's worse than Zcmp because those functions sometimes push/pop more registers than the exact number that was asked for. At least Zcmp push/pop doesn't have this issue.

This will be nasty to fix.

[wan-yuqi]

It wouldn't be compatible with -msave-restore or the Zcmp push/pop instructions.

Yeah, I have tested the -msave-restore option and found that GCC also has similar issues.

The Reason is the save/restore built-in functions (defined in save.S) used to replace prologue/epilogue need consider stack 16byte-alignment and save all callee-saved register (start from X1) in order. However, any of the register may be used as global register variable. It seems unavoidable. I havent come up with a good idea to the problem.

[lenary]

As there's no way we can know that you might have modified your save/restore routines to match this option, so I think we just have to assume that they might push all the CSRs, and avoid using them.

There might still be some opportunities to use push/pop for the unreserved prefix of the callee-saves but I think that would make generating the CFI information much more difficult. Frankly, I think the only thing that's maintainable and justifiable is to just fall back to emitting the saves/restores manually, and you'll have to cope with any code size impact - if it's too much for you, find a different register to reserve.