[RISCV] Fix the CFI offset for callee-saved registers stored by Zcmp push.

[Jim]

Summary

Issue mentioned: https://github.com/riscv/riscv-code-size-reduction/issues/182

The order of callee-saved registers stored by Zcmp push in memory is reversed.

Pseudo code for cm.push in https://github.com/riscv/riscv-code-size-reduction/releases/download/v1.0.4-1/Zc.1.0.4-1.pdf

if (XLEN==32) bytes=4; else bytes=8;

addr=sp-bytes;
for(i in 27,26,25,24,23,22,21,20,19,18,9,8,1)  {
  //if register i is in xreg_list
  if (xreg_list[i]) {
    switch(bytes) {
      4:  asm("sw x[i], 0(addr)");
      8:  asm("sd x[i], 0(addr)");
    }
    addr-=bytes;
  }
}

The placement order for push is s11, s10, ..., ra.

CFI offset should be calculed as reversed order for correct stack unwinding.

[Jim]

Fix comment

[KYG]

Maybe slightly shorter title? E.g., "Fix the CFI offset for callee-saved registers stored by Zcmp push/pop".

Otherwise LGTM, but wait for other reviewer's approve.

[fakepaper56]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

587

The original code looks like we expect negative frame indices are related to their saved location.
If we want to reserve the behavior of negative frame indices, I think we should have specific hasReservedSpillSlot behavior for Zcmp?

[Jim]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

587

I am not sure does any other take care about the order of callee-saved register stored in memory except CFI offset.

RISCVRegisterInfo::hasReservedSpillSlot now is unable to get the total number of callee-saved register stored for calculating frame index in reverse order. I would try to pass extra parameter number of callee-saved regsiter saved to RISCVRegisterInfo::hasReservedSpillSlot.

[kito-cheng]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

585–586

Drop this two line, it might cause confusion IMO.

[fakepaper56]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

583–584

Maybe add curly bracket here to make it more readable?

587

I am not sure does any other take care about the order of callee-saved register stored in memory except CFI offset.

I think you are right. Actually, my concern was that it increases the complexity of this piece of code. But I forgot it is hard for hasReservedSpillSlot to find out the sp offset when Zcmp enabled.
This part is good for me now and thank you for your feedback .

[Jim]

Address comments.

[fakepaper56]

LGTM.

[craig.topper]

Does this need to be backported to LLVM 17 branch?

[kito-cheng]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

583–584

Nit:

I think this need more curly brackets around there:

if (FrameIdx < 0) {
  if (RVFI->isPushable(MF)) {
    ...
  } else {
    ...
  }
} else {
  ...
}

[Jim]

Address @kito-cheng's comment.

[kito-cheng]

LGTM, and I think this should go to LLVM 17 branch, so don't forgot to create github issue for backport :)

[Jim]

In D156437#4552514, @kito-cheng wrote:

LGTM, and I think this should go to LLVM 17 branch, so don't forgot to create github issue for backport :)

Thanks. I created a issue https://github.com/llvm/llvm-project/issues/64330.

[Jim]

https://github.com/llvm/llvm-project-release-prs/pull/513