This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Jim on Jul 27 2023, 7:36 AM.

Details

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.

Diff Detail

Event Timeline

Jim created this revision.Jul 27 2023, 7:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 7:36 AM
Jim requested review of this revision.Jul 27 2023, 7:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 7:36 AM
Jim updated this revision to Diff 544772.Jul 27 2023, 7:38 AM

Fix comment

KYG added a comment.Jul 27 2023, 7:25 PM

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.

Jim retitled this revision from [RISCV] Reverse CFI offset order for callee-saved registers stored by Zcmp push for correct stack unwinding. to [RISCV] Fix the CFI offset for callee-saved registers stored by Zcmp push/pop..Jul 27 2023, 7:50 PM
fakepaper56 added inline comments.Jul 31 2023, 2:01 AM
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 retitled this revision from [RISCV] Fix the CFI offset for callee-saved registers stored by Zcmp push/pop. to [RISCV] Fix the CFI offset for callee-saved registers stored by Zcmp push..Jul 31 2023, 2:22 AM
Jim added inline comments.Jul 31 2023, 2:39 AM
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 added inline comments.Jul 31 2023, 7:00 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
585–586

Drop this two line, it might cause confusion IMO.

fakepaper56 added inline comments.Jul 31 2023, 8:13 AM
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 updated this revision to Diff 545889.Jul 31 2023, 6:03 PM

Address comments.

Jim marked 4 inline comments as done.Jul 31 2023, 6:04 PM
This revision is now accepted and ready to land.Jul 31 2023, 8:12 PM

Does this need to be backported to LLVM 17 branch?

kito-cheng added inline comments.Aug 1 2023, 5:14 PM
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 updated this revision to Diff 546291.Aug 1 2023, 6:54 PM

Address @kito-cheng's comment.

kito-cheng accepted this revision.Aug 1 2023, 7:13 PM

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

Jim added a comment.Aug 1 2023, 10:16 PM

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.