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

[luismarques]

Summary

When using the split sp adjustment and using the frame-pointer we were still emitting CFI CFA directives based on the sp value. The final sp-based offset also didn't reflect the two-stage sp adjust. There remain CFI issues that aren't related to the split sp adjustment, and thus will be addressed in a separate patch.

[shiva0217]

Hi @luismarques, thanks for fixing this.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

198

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?

267

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);
}

290

Should take variable arguments size into account
Somthing like:
FirstSPAdjustAmount ? -FirstSPAdjustAmount + RVFI->getVarArgsSaveSize(): -FPOffset

[luismarques]

Address @shiva0217's concerns.

[luismarques]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

267

I would prefer to do codegen improvements in a separate patch. Thanks for the suggestion!

[shiva0217]

Hi @luismarques, thanks for the patch, LGTM.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

267

Ok, that makes sense.

[lenary]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

291

Is there a varargs-based test for this patch?

[luismarques]

Add test changes based on D69721.

[luismarques]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

291

There is now. Good catch! :)

[lenary]

The CFI directives look a lot more sane after this diff.

LGTM, I think this can be landed. We'll carefully watch the CI.

[luismarques]

Rebased to match changes to D69721.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
 
   // Emit the second SP adjustment after saving callee saved registers.
   if (FirstSPAdjustAmount) {
     uint64_t SecondSPAdjustAmount = MFI.getStackSize() - FirstSPAdjustAmount;
     assert(SecondSPAdjustAmount > 0 &&
            "SecondSPAdjustAmount should be greater than zero");
     adjustReg(MBB, MBBI, DL, SPReg, SPReg, -SecondSPAdjustAmount,
               MachineInstr::FrameSetup);
+    if (!hasFP(MF)) {
       // 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)) {
     // Realign Stack
     const RISCVRegisterInfo *RI = STI.getRegisterInfo();
     if (RI->needsStackRealignment(MF)) {
       unsigned MaxAlignment = MFI.getMaxAlignment();
 
       const RISCVInstrInfo *TII = STI.getInstrInfo();
void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
   }
 
   uint64_t FirstSPAdjustAmount = getFirstSPAdjustAmount(MF);
   if (FirstSPAdjustAmount) {
     uint64_t SecondSPAdjustAmount = MFI.getStackSize() - FirstSPAdjustAmount;
     assert(SecondSPAdjustAmount > 0 &&
            "SecondSPAdjustAmount should be greater than zero");
 
     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))
+    if (!hasFP(MF)) {
+      unsigned CFIIndex = MF.addFrameInst(
+          MCCFIInstruction::createDefCfaOffset(nullptr, -FirstSPAdjustAmount));
+      BuildMI(MBB, LastFrameDestroy, DL,
+              TII->get(TargetOpcode::CFI_INSTRUCTION))
           .addCFIIndex(CFIIndex);
     }
+  }
 
   if (hasFP(MF)) {
     // To find the instruction restoring FP from stack.
     for (auto &I = LastFrameDestroy; I != MBBI; ++I) {
       if (I->mayLoad() && I->getOperand(0).isReg()) {
         Register DestReg = I->getOperand(0).getReg();
         if (DestReg == FPReg) {
           // If there is frame pointer, after restoring $fp registers, we
-          // need adjust CFA to ($sp - FPOffset).
-          // Emit ".cfi_def_cfa $sp, -FPOffset"
+          // need adjust CFA back to the correct sp-based offset.
+          // Emit ".cfi_def_cfa $sp, CFAOffset"
+          uint64_t CFAOffset =
+              FirstSPAdjustAmount ? -FirstSPAdjustAmount : -FPOffset;
           unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfa(
-              nullptr, RI->getDwarfRegNum(SPReg, true), -FPOffset));
+              nullptr, RI->getDwarfRegNum(SPReg, true), CFAOffset));
           BuildMI(MBB, std::next(I), DL,
                   TII->get(TargetOpcode::CFI_INSTRUCTION))
               .addCFIIndex(CFIIndex);
           break;
         }
       }
     }
   }

llvm/test/CodeGen/RISCV/large-stack.ll

 ; RV32I-WITHFP-NEXT:    sw s0, 2024(sp)
 ; RV32I-WITHFP-NEXT:    .cfi_offset ra, -4
 ; RV32I-WITHFP-NEXT:    .cfi_offset s0, -8
 ; RV32I-WITHFP-NEXT:    addi s0, sp, 2032
 ; RV32I-WITHFP-NEXT:    .cfi_def_cfa s0, 0
 ; RV32I-WITHFP-NEXT:    lui a0, 74565
 ; RV32I-WITHFP-NEXT:    addi a0, a0, -352
 ; RV32I-WITHFP-NEXT:    sub sp, sp, a0
-; RV32I-WITHFP-NEXT:    .cfi_def_cfa_offset 305419920
 ; RV32I-WITHFP-NEXT:    lui a0, 74565
 ; RV32I-WITHFP-NEXT:    addi a0, a0, -352
 ; RV32I-WITHFP-NEXT:    add sp, sp, a0
-; RV32I-WITHFP-NEXT:    .cfi_def_cfa_offset 2032
 ; RV32I-WITHFP-NEXT:    lw s0, 2024(sp)
-; RV32I-WITHFP-NEXT:    .cfi_def_cfa sp, 305419920
+; RV32I-WITHFP-NEXT:    .cfi_def_cfa sp, 2032
 ; RV32I-WITHFP-NEXT:    lw ra, 2028(sp)
 ; RV32I-WITHFP-NEXT:    .cfi_restore ra
 ; RV32I-WITHFP-NEXT:    .cfi_restore s0
 ; RV32I-WITHFP-NEXT:    addi sp, sp, 2032
 ; RV32I-WITHFP-NEXT:    .cfi_def_cfa_offset 0
 ; RV32I-WITHFP-NEXT:    ret
   %tmp = alloca [ 305419896 x i8 ] , align 4
   ret void

 ; RV32I-WITHFP-NEXT:    .cfi_offset s0, -8
 ; RV32I-WITHFP-NEXT:    .cfi_offset s1, -12
 ; RV32I-WITHFP-NEXT:    .cfi_offset s2, -16
 ; RV32I-WITHFP-NEXT:    addi s0, sp, 2032
 ; RV32I-WITHFP-NEXT:    .cfi_def_cfa s0, 0
 ; RV32I-WITHFP-NEXT:    lui a1, 97
 ; RV32I-WITHFP-NEXT:    addi a1, a1, 688
 ; RV32I-WITHFP-NEXT:    sub sp, sp, a1
-; RV32I-WITHFP-NEXT:    .cfi_def_cfa_offset 400032
 ; RV32I-WITHFP-NEXT:    lui a1, 78
 ; RV32I-WITHFP-NEXT:    addi a1, a1, 512
 ; RV32I-WITHFP-NEXT:    lui a2, 1048478
 ; RV32I-WITHFP-NEXT:    addi a2, a2, 1388
 ; RV32I-WITHFP-NEXT:    add a2, s0, a2
 ; RV32I-WITHFP-NEXT:    mv a2, a2
 ; RV32I-WITHFP-NEXT:    add a1, a2, a1
 ; RV32I-WITHFP-NEXT:    #APP
 ; RV32I-WITHFP-NEXT:    nop
 ; RV32I-WITHFP-NEXT:    #NO_APP
 ; RV32I-WITHFP-NEXT:    sw a0, 0(a1)
 ; RV32I-WITHFP-NEXT:    #APP
 ; RV32I-WITHFP-NEXT:    nop
 ; RV32I-WITHFP-NEXT:    #NO_APP
 ; RV32I-WITHFP-NEXT:    lui a0, 97
 ; RV32I-WITHFP-NEXT:    addi a0, a0, 688
 ; RV32I-WITHFP-NEXT:    add sp, sp, a0
-; RV32I-WITHFP-NEXT:    .cfi_def_cfa_offset 2032
 ; RV32I-WITHFP-NEXT:    lw s2, 2016(sp)
 ; RV32I-WITHFP-NEXT:    lw s1, 2020(sp)
 ; RV32I-WITHFP-NEXT:    lw s0, 2024(sp)
-; RV32I-WITHFP-NEXT:    .cfi_def_cfa sp, 400032
+; RV32I-WITHFP-NEXT:    .cfi_def_cfa sp, 2032
 ; RV32I-WITHFP-NEXT:    lw ra, 2028(sp)
 ; RV32I-WITHFP-NEXT:    .cfi_restore ra
 ; RV32I-WITHFP-NEXT:    .cfi_restore s0
 ; RV32I-WITHFP-NEXT:    .cfi_restore s1
 ; RV32I-WITHFP-NEXT:    .cfi_restore s2
 ; RV32I-WITHFP-NEXT:    addi sp, sp, 2032
 ; RV32I-WITHFP-NEXT:    .cfi_def_cfa_offset 0
 ; RV32I-WITHFP-NEXT:    ret