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

[Yeting Kuo via Phabricator via llvm-commits]

fakepaper56 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:585
     if (FrameIdx < 0)
-      Offset = FrameIdx * (int64_t) STI.getXLen() / 8;
+      if (RVFI->isPushable(MF))
+        // Callee-saved register stored by Zcmp push is in reverse order.
----------------
Maybe add curly bracket here to make it more readable?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:589
+        // .cfi_offset s0, -8 => .cfi_offset s0, -4
+        Offset = -(FrameIdx + RVFI->getRVPushRegs() + 1) *
+                 (int64_t)STI.getXLen() / 8;
----------------
Jim wrote:
> fakepaper56 wrote:
> > 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`?
> 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.
> 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 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156437/new/

https://reviews.llvm.org/D156437