This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by luismarques on Oct 24 2019, 6:59 AM.



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.

Diff Detail

Event Timeline

luismarques created this revision.Oct 24 2019, 6:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2019, 6:59 AM
shiva0217 requested changes to this revision.Oct 27 2019, 10:30 PM

Hi @luismarques, thanks for fixing this.


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?


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,

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

This revision now requires changes to proceed.Oct 27 2019, 10:30 PM

Address @shiva0217's concerns.

luismarques marked 4 inline comments as done.Oct 31 2019, 2:13 AM
luismarques added inline comments.

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

shiva0217 accepted this revision.Oct 31 2019, 4:46 AM

Hi @luismarques, thanks for the patch, LGTM.


Ok, that makes sense.

This revision is now accepted and ready to land.Oct 31 2019, 4:46 AM
luismarques edited the summary of this revision. (Show Details)Oct 31 2019, 11:37 AM
lenary added inline comments.Nov 1 2019, 3:40 AM

Is there a varargs-based test for this patch?

luismarques marked an inline comment as done.

Add test changes based on D69721.

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

There is now. Good catch! :)

lenary accepted this revision.Nov 4 2019, 5:11 AM

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 marked an inline comment as done.

Rebased to match changes to D69721.

This revision was automatically updated to reflect the committed changes.