This is an archive of the discontinued LLVM Phabricator instance.

Differential D73862

[RISCV] Fix incorrect FP base CFI offset for variable argument functions
ClosedPublic

Authored by shiva0217 on Feb 2 2020, 10:12 PM.

Details

Reviewers
asb
lenary
luismarques
Commits
rG43ca67c05d28: [RISCV] Fix incorrect FP base CFI offset for variable argument functions
rG64f417200e10: [RISCV] Fix incorrect FP base CFI offset for variable argument functions
Summary

When the FP exists, the FP base CFI directive offset should take the size of variable arguments into account.

Diff Detail

Event Timeline

shiva0217 created this revision.Feb 2 2020, 10:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2020, 10:12 PM
Herald added subscribers: apazos, sameer.abuasal, pzheng and 22 others. · View Herald Transcript
luismarques added a comment.Feb 7 2020, 8:35 AM

LGTM (just tweak the comment).

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
184

This comment is lacking the - of the code below. I understand that the value emitted is positive, but the way the comment is written it's inconsistent with the implementation.

Herald added a subscriber: evandro. · View Herald TranscriptFeb 7 2020, 8:35 AM
luismarques accepted this revision.Feb 7 2020, 8:37 AM
This revision is now accepted and ready to land.Feb 7 2020, 8:37 AM
shiva0217 updated this revision to Diff 243468.Feb 9 2020, 7:26 PM

Update the patch to address the comment.

Hi @luismarques, thanks for the review.

Closed by commit rG64f417200e10: [RISCV] Fix incorrect FP base CFI offset for variable argument functions (authored by shiva0217). · Explain WhyFeb 9 2020, 7:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
Target/
RISCV/
4 lines
test/
CodeGen/
RISCV/
8 lines

Diff 243470

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

Show First 20 Lines • Show All 175 Lines • ▼ Show 20 Linesvoid RISCVFrameLowering::emitPrologue(MachineFunction &MF,
if (hasFP(MF)) { if (hasFP(MF)) {
if (STI.isRegisterReservedByUser(FPReg)) if (STI.isRegisterReservedByUser(FPReg))
MF.getFunction().getContext().diagnose(DiagnosticInfoUnsupported{ MF.getFunction().getContext().diagnose(DiagnosticInfoUnsupported{
MF.getFunction(), "Frame pointer required, but has been reserved."}); MF.getFunction(), "Frame pointer required, but has been reserved."});
adjustReg(MBB, MBBI, DL, FPReg, SPReg, adjustReg(MBB, MBBI, DL, FPReg, SPReg,
StackSize - RVFI->getVarArgsSaveSize(), MachineInstr::FrameSetup); StackSize - RVFI->getVarArgsSaveSize(), MachineInstr::FrameSetup);
// Emit ".cfi_def_cfa $fp, 0" // Emit ".cfi_def_cfa $fp, -RVFI->getVarArgsSaveSize()"
luismarquesUnsubmitted
Not Done
ReplyInline Actions

This comment is lacking the - of the code below. I understand that the value emitted is positive, but the way the comment is written it's inconsistent with the implementation.

luismarques: This comment is lacking the `-` of the code below. I understand that the value emitted is…
unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfa( unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfa(
nullptr, RI->getDwarfRegNum(FPReg, true), 0)); nullptr, RI->getDwarfRegNum(FPReg, true), -RVFI->getVarArgsSaveSize()));
BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION)) BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex); .addCFIIndex(CFIIndex);
} }
// Emit the second SP adjustment after saving callee saved registers. // Emit the second SP adjustment after saving callee saved registers.
if (FirstSPAdjustAmount) { if (FirstSPAdjustAmount) {
uint64_t SecondSPAdjustAmount = MFI.getStackSize() - FirstSPAdjustAmount; uint64_t SecondSPAdjustAmount = MFI.getStackSize() - FirstSPAdjustAmount;
assert(SecondSPAdjustAmount > 0 && assert(SecondSPAdjustAmount > 0 &&
▲ Show 20 Lines • Show All 292 Lines • Show Last 20 Lines

llvm/test/CodeGen/RISCV/vararg.ll

Show First 20 Lines • Show All 58 Lines • ▼ Show 20 Lines
; ILP32-ILP32F-WITHFP: # %bb.0: ; ILP32-ILP32F-WITHFP: # %bb.0:
; ILP32-ILP32F-WITHFP-NEXT: addi sp, sp, -48 ; ILP32-ILP32F-WITHFP-NEXT: addi sp, sp, -48
; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa_offset 48 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa_offset 48
; ILP32-ILP32F-WITHFP-NEXT: sw ra, 12(sp) ; ILP32-ILP32F-WITHFP-NEXT: sw ra, 12(sp)
; ILP32-ILP32F-WITHFP-NEXT: sw s0, 8(sp) ; ILP32-ILP32F-WITHFP-NEXT: sw s0, 8(sp)
; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset ra, -36 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset ra, -36
; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset s0, -40 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset s0, -40
; ILP32-ILP32F-WITHFP-NEXT: addi s0, sp, 16 ; ILP32-ILP32F-WITHFP-NEXT: addi s0, sp, 16
; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa s0, 0 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa s0, 32
; ILP32-ILP32F-WITHFP-NEXT: mv a0, a1 ; ILP32-ILP32F-WITHFP-NEXT: mv a0, a1
; ILP32-ILP32F-WITHFP-NEXT: sw a7, 28(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a7, 28(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a6, 24(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a6, 24(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a5, 20(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a5, 20(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a4, 16(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a4, 16(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a3, 12(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a3, 12(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a2, 8(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a2, 8(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a1, 4(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a1, 4(s0)
▲ Show 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
; LP64-LP64F-LP64D-WITHFP: # %bb.0: ; LP64-LP64F-LP64D-WITHFP: # %bb.0:
; LP64-LP64F-LP64D-WITHFP-NEXT: addi sp, sp, -96 ; LP64-LP64F-LP64D-WITHFP-NEXT: addi sp, sp, -96
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa_offset 96 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa_offset 96
; LP64-LP64F-LP64D-WITHFP-NEXT: sd ra, 24(sp) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd ra, 24(sp)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd s0, 16(sp) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd s0, 16(sp)
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset ra, -72 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset ra, -72
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset s0, -80 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset s0, -80
; LP64-LP64F-LP64D-WITHFP-NEXT: addi s0, sp, 32 ; LP64-LP64F-LP64D-WITHFP-NEXT: addi s0, sp, 32
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa s0, 0 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa s0, 64
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a1, 8(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a1, 8(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a7, 56(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a7, 56(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a6, 48(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a6, 48(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a5, 40(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a5, 40(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a4, 32(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a4, 32(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a3, 24(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a3, 24(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a2, 16(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a2, 16(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: addi a0, s0, 8 ; LP64-LP64F-LP64D-WITHFP-NEXT: addi a0, s0, 8
▲ Show 20 Lines • Show All 1,668 Lines • ▼ Show 20 Lines
; ILP32-ILP32F-WITHFP: # %bb.0: ; ILP32-ILP32F-WITHFP: # %bb.0:
; ILP32-ILP32F-WITHFP-NEXT: addi sp, sp, -2032 ; ILP32-ILP32F-WITHFP-NEXT: addi sp, sp, -2032
; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa_offset 2032 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa_offset 2032
; ILP32-ILP32F-WITHFP-NEXT: sw ra, 1996(sp) ; ILP32-ILP32F-WITHFP-NEXT: sw ra, 1996(sp)
; ILP32-ILP32F-WITHFP-NEXT: sw s0, 1992(sp) ; ILP32-ILP32F-WITHFP-NEXT: sw s0, 1992(sp)
; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset ra, -36 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset ra, -36
; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset s0, -40 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_offset s0, -40
; ILP32-ILP32F-WITHFP-NEXT: addi s0, sp, 2000 ; ILP32-ILP32F-WITHFP-NEXT: addi s0, sp, 2000
; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa s0, 0 ; ILP32-ILP32F-WITHFP-NEXT: .cfi_def_cfa s0, 32
; ILP32-ILP32F-WITHFP-NEXT: lui a0, 24414 ; ILP32-ILP32F-WITHFP-NEXT: lui a0, 24414
; ILP32-ILP32F-WITHFP-NEXT: addi a0, a0, -1728 ; ILP32-ILP32F-WITHFP-NEXT: addi a0, a0, -1728
; ILP32-ILP32F-WITHFP-NEXT: sub sp, sp, a0 ; ILP32-ILP32F-WITHFP-NEXT: sub sp, sp, a0
; ILP32-ILP32F-WITHFP-NEXT: mv a0, a1 ; ILP32-ILP32F-WITHFP-NEXT: mv a0, a1
; ILP32-ILP32F-WITHFP-NEXT: sw a7, 28(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a7, 28(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a6, 24(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a6, 24(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a5, 20(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a5, 20(s0)
; ILP32-ILP32F-WITHFP-NEXT: sw a4, 16(s0) ; ILP32-ILP32F-WITHFP-NEXT: sw a4, 16(s0)
▲ Show 20 Lines • Show All 111 Lines • ▼ Show 20 Lines
; LP64-LP64F-LP64D-WITHFP: # %bb.0: ; LP64-LP64F-LP64D-WITHFP: # %bb.0:
; LP64-LP64F-LP64D-WITHFP-NEXT: addi sp, sp, -2032 ; LP64-LP64F-LP64D-WITHFP-NEXT: addi sp, sp, -2032
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa_offset 2032 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa_offset 2032
; LP64-LP64F-LP64D-WITHFP-NEXT: sd ra, 1960(sp) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd ra, 1960(sp)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd s0, 1952(sp) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd s0, 1952(sp)
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset ra, -72 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset ra, -72
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset s0, -80 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_offset s0, -80
; LP64-LP64F-LP64D-WITHFP-NEXT: addi s0, sp, 1968 ; LP64-LP64F-LP64D-WITHFP-NEXT: addi s0, sp, 1968
; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa s0, 0 ; LP64-LP64F-LP64D-WITHFP-NEXT: .cfi_def_cfa s0, 64
; LP64-LP64F-LP64D-WITHFP-NEXT: lui a0, 24414 ; LP64-LP64F-LP64D-WITHFP-NEXT: lui a0, 24414
; LP64-LP64F-LP64D-WITHFP-NEXT: addiw a0, a0, -1680 ; LP64-LP64F-LP64D-WITHFP-NEXT: addiw a0, a0, -1680
; LP64-LP64F-LP64D-WITHFP-NEXT: sub sp, sp, a0 ; LP64-LP64F-LP64D-WITHFP-NEXT: sub sp, sp, a0
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a1, 8(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a1, 8(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a7, 56(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a7, 56(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a6, 48(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a6, 48(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a5, 40(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a5, 40(s0)
; LP64-LP64F-LP64D-WITHFP-NEXT: sd a4, 32(s0) ; LP64-LP64F-LP64D-WITHFP-NEXT: sd a4, 32(s0)
Show All 28 Lines