[PATCH] D69385: [RISCV] Fix CFA when doing split sp adjustment with fp

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 requested changes to this revision.
shiva0217 added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: sameer.abuasal.

Hi @luismarques, thanks for fixing this.



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:198
               MachineInstr::FrameSetup);
-    // Emit ".cfi_def_cfa_offset StackSize"
-    unsigned CFIIndex = MF.addFrameInst(
-        MCCFIInstruction::createDefCfaOffset(nullptr, -MFI.getStackSize()));
-    BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-        .addCFIIndex(CFIIndex);
+    if (!hasFP(MF)) {
+      // Emit ".cfi_def_cfa_offset StackSize"
----------------
Could you add a comment before `if(!hasFP(MF))` to describe that we don't need to generate `.cfi_def_cfa_offset` for SP adjustment after setting FP because the CFA calculation will base on FP after `.cfi_def_cfa s0, 0` emission?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:267
 
     adjustReg(MBB, LastFrameDestroy, DL, SPReg, SPReg, SecondSPAdjustAmount,
               MachineInstr::FrameDestroy);
----------------
It seems that we might be able to reduce the epilogue instructions if the FP exists. Something like:

  
  if (hasFP(MF) && !isInt<12>(SecondSPAdjustAmount)) {
    adjustReg(MBB, LastFrameDestroy, DL, SPReg, FPReg,-FirstSPAdjustAmount + RVFI->getVarArgsSaveSize(), MachineInstr::FrameDestroy);
  } else {
    adjustReg(MBB, LastFrameDestroy, DL, SPReg, SPReg, SecondSPAdjustAmount, MachineInstr::FrameDestroy);
      // Emit ".cfi_def_cfa_offset FirstSPAdjustAmount"
      unsigned CFIIndex = MF.addFrameInst(
          MCCFIInstruction::createDefCfaOffset(nullptr, -FirstSPAdjustAmount));
      BuildMI(MBB, LastFrameDestroy, DL,
              TII->get(TargetOpcode::CFI_INSTRUCTION))
          .addCFIIndex(CFIIndex);
  }


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:290
+          uint64_t CFAOffset =
+              FirstSPAdjustAmount ? -FirstSPAdjustAmount : -FPOffset;
           unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfa(
----------------
Should take variable arguments size into account
Somthing like:
FirstSPAdjustAmount ? -FirstSPAdjustAmount + RVFI->getVarArgsSaveSize(): -FPOffset


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69385