[PATCH] D68011: [RISCV] Split SP adjustment to reduce the offset of callee saved register spill and restore

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 marked 2 inline comments as done.
shiva0217 added inline comments.


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:245
+  uint64_t SecondSPAdjustAmount = MFI.getStackSize() - getFirstSPAdjustAmount();
+  if (isSplitSPAdjust(MF) && (SecondSPAdjustAmount > 0)) {
+    adjustReg(MBB, LastFrameDestroy, DL, SPReg, SPReg, SecondSPAdjustAmount,
----------------
luismarques wrote:
> If `SecondSPAdjustmentAmount == 0` then haven't we done something wrong? The first SP adjustment becomes the only one, so `isSplitSPAdjust` can't logically be true, right? If you keep the separate boolean/amount, then I would suggest asserting that `SecondSPAdjustmentAmount > 0` when `isSplitSPAdjust` is true.
When the `isSplitSPAdjust` return true, the `StackSize > 2048`, and the `FirstSPAdjustAmount = 2048 - StackAlign`. So `SecondSPAdjustmentAmount = StackSize - FirstSPAdjustAmount` should always greater than zero when  `isSplitSPAdjust` return true. We probably could remove the `SecondSPAdjustAmount > 0` in the if line. I think it's great idea to add assertion for `SecondSPAdjustmentAmount > 0` to catch anything we unexpect even if we merge the functions, thanks.


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.h:49
+  // Determine to split the SP adjustment of prologue/epilogue.
+  bool isSplitSPAdjust(const MachineFunction &MF) const;
+
----------------
luismarques wrote:
> I'm not sure we need to split things into the boolean and the adjustment amount. Why not just have a single function, and if the returned initial adjustment is 0 then we just do the normal SP adjustment, otherwise we do it in two steps?
Good idea, I'll merge the functions, thanks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68011