[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG07f7c00208b3: [RISCV] Add support for save/restore of callee-saved registers via libcalls (authored by lewis-revill).

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added a comment. Since the DebugInfo fix has been accepted, I'm looking to get this patch and that fix committed shortly if there are no problems caused by rebasing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 242723. lewis-revill added a comment. Rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files: clang/lib/Driver/ToolChains/Arch/RISCV.cpp

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added a comment. In D62686#1820816 , @apazos wrote: > Lewis, your latest patch looks good, we just had another run with no new > failures. But we know it will have issues with -g. So I think we should not > merge it yet. Do you have a

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Lewis, your latest patch looks good, we just had another run with no new failures. But we know it will have issues with -g. So I think we should not merge it yet. Do you have a version of the patch that creates the labels for the compiler-generated save/restore lib

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added a comment. Should I wait for the comments to be resolved on D71593 before I commit this patch? Ideally if this patch makes it into a release then that bug fix should be there too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

pzheng added a comment. In D62686#1816681 , @lewis-revill wrote: > Fix .cfi_offset signedness error. Thanks, @lewis-revill. It looks correct now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added a comment. In D62686#1808041 , @apazos wrote: > Lewis, is the patch final? It would be good to merge it before the 10.0 > release branch creation on Jan 15th I would say so now. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 237620. lewis-revill added a comment. Fix .cfi_offset signedness error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files:

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added a comment. In D62686#1815158 , @pzheng wrote: > I see the following .cfi_offset directives generated using @shiva0217's test > case. Any idea why the offset for ra is 536870908? > > callt0, __riscv_save_0 > .cfi_def_cfa_offset

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

pzheng added a comment. I see the following .cfi_offset directives generated using @shiva0217's test case. Any idea why the offset for ra is 536870908? callt0, __riscv_save_0 .cfi_def_cfa_offset 16 .cfi_offset ra, 536870908 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Lewis, is the patch final? It would be good to merge it before the 10.0 release branch creation on Jan 15th Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 234853. lewis-revill added a comment. Added .cfi_offset directives for registers saved by libcalls. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files:

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 234842. lewis-revill added a comment. Fixed existing .cfi_offset offsets. Since these are frame-pointer based they also need to account for the libcall stack adjustment. Currently working on adding .cfi_offset instructions for the registers saved by

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 234091. lewis-revill added a comment. Rebased and addressed StackSize vs RealStackSize error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files:

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

jrtc27 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:668 +.setMIFlag(MachineInstr::FrameSetup); + +// Add registers spilled in libcall as liveins. lewis-revill wrote: > shiva0217 wrote: > > GCC will generate stack

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill marked an inline comment as done. lewis-revill added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:289 unsigned CFIIndex = MF.addFrameInst( MCCFIInstruction::createDefCfaOffset(nullptr, -StackSize)); BuildMI(MBB, MBBI, DL,

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill marked an inline comment as done. lewis-revill added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:667 +.addExternalSymbol(SpillLibCall, RISCVII::MO_CALL) +.setMIFlag(MachineInstr::FrameSetup); + shiva0217

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

shiva0217 added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:289 unsigned CFIIndex = MF.addFrameInst( MCCFIInstruction::createDefCfaOffset(nullptr, -StackSize)); BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 232702. lewis-revill added a comment. Rebased. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files: clang/lib/Driver/ToolChains/Arch/RISCV.cpp

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Lewis, try rebasing it, not applying cleanly nor https://reviews.llvm.org/D62190 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 ___

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:278 // Add CFI directives for callee-saved registers. - const std::vector = MFI.getCalleeSavedInfo(); - // Iterate over list of callee-saved registers and emit .cfi_restore - //

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 229485. lewis-revill added a comment. Rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files: clang/lib/Driver/ToolChains/Arch/RISCV.cpp

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

pzheng added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:278 // Add CFI directives for callee-saved registers. - const std::vector = MFI.getCalleeSavedInfo(); - // Iterate over list of callee-saved registers and emit .cfi_restore - //

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 227512. lewis-revill added a comment. Herald added a subscriber: sameer.abuasal. Rebased and merged D68644 into this patch - this patch already assumes shrink wrapping support anyway. Repository: rG LLVM Github

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 226166. lewis-revill added a comment. Rebase on top of shrink wrapping patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files:

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Thanks Lewis, the runs are looking good, no failures, and good code size savings (average 3%) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 225230. lewis-revill added a comment. Disable the save/restore optimization when a function contains tail calls. Address various miscellaneous concerns with the patch. Update tests to include less redundant code when checking cases where save/restore

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. > Is it worth trying to disallow tail call optimization completely if this flag > is enabled? I'm not sure what GCC does exactly. but this seems to be the > behaviour. I had reported above that I have already run that test: with "-fno-optimize-sibling-calls

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added inline comments. Comment at: llvm/test/CodeGen/RISCV/saverestore.ll:348 + +; Check that functions with varargs do not use save/restore code + luismarques wrote: > Maybe for these tests just put a -NOT check that __riscv_save_ isn't called?

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill added a comment. In D62686#1708792 , @apazos wrote: > Yes Eli thanks for pointing out there are more scenarios that can fail. > It looks like the best solution is to permit both flags on, but then bail > out from doing this transformation

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Yes Eli thanks for pointing out there are more scenarios that can fail. It looks like the best solution is to permit both flags on, but then bail out from doing this transformation if there is any type of tail call already in the function. This way we avoid messing with

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

efriedma added a comment. > Bug2: test case provided above. Please Lewis take a look at how this case can > be fixed. Isn't the issue just that the code is checking for PseudoTAIL, and not PseudoTAILIndirect? What happens if a tail call has more than 8 arguments? What happens for a

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Here is the bugpoint-reduced test case for the SPEC failure when enabling -msave-restore and allowing tail calls: Run the command llc test.ll -mattr=+save-restore -o out.s You will see the code generated is wrong: tail__riscv_restore_2 jr a5 target

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

luismarques added a comment. The priority for this patch is to address the issues reported by @apazos but after that please check the clang-format output. There are some cases in this patch where it might make sense to use a different formatting than clang-format indicates, but the remaining

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 223855. lewis-revill added a comment. Rebased to fix conflicts with recent split SP adjustments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 Files:

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:631 + MachineBasicBlock::iterator NewMI = + BuildMI(MBB, MI, DL, TII.get(RISCV::PseudoCALL)) + .add(MI->getOperand(0)); apazos wrote: > Where are we

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:631 + MachineBasicBlock::iterator NewMI = + BuildMI(MBB, MI, DL, TII.get(RISCV::PseudoCALL)) + .add(MI->getOperand(0)); Where are we making sure the

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Lewis, with this patch we see less failures. But still some tests in SPEC and perennial test suites are failing. Pengxuan and I are trying to triage the failures. Here is what we see in one of the failed tests: Code right before rologue/Epilogue Insertion & Frame

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

apazos added a comment. Thanks for the patch update. I will launch some new correctness runs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/ https://reviews.llvm.org/D62686 ___ cfe-commits

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 222612. lewis-revill added a comment. Rewrote logic to calculate stack sizes, frame indexes and frame pointer offsets. This was necessary to take into account the fact that the save/restore lib calls are essentially an opaque section of the stack that

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill planned changes to this revision. lewis-revill added a comment. It seems like the regressions I'm seeing are due to the fact that calculating offsets for fixed objects at the top of the frame didn't account for extra stack size adjustment from the libcalls. I'm trying to find a

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill marked an inline comment as done. lewis-revill added a comment. In D62686#1675347 , @lenary wrote: > We discussed this in the RISC-V meeting on 19 Sept 2019. @apazos says there > are some SPEC failures in both 2006 and 2017, which would be

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lenary added inline comments. Comment at: llvm/lib/Target/RISCV/RISCV.td:72 +def FeatureSaveRestore : SubtargetFeature<"save-restore", "EnableSaveRestore", + "true", "Enable save/restore.">; lewis-revill wrote: > lenary

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill marked 2 inline comments as done. lewis-revill added inline comments. Comment at: llvm/lib/Target/RISCV/RISCV.td:72 +def FeatureSaveRestore : SubtargetFeature<"save-restore", "EnableSaveRestore", + "true", "Enable

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lenary added a comment. Two nits, that I wanted to submit before the meeting, but didn't get around to. Comment at: llvm/lib/Target/RISCV/RISCV.td:72 +def FeatureSaveRestore : SubtargetFeature<"save-restore", "EnableSaveRestore", +

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lenary added a comment. We discussed this in the RISC-V meeting on 19 Sept 2019. @apazos says there are some SPEC failures in both 2006 and 2017, which would be good to triage. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62686/new/

[PATCH] D62686: [RISCV] Add support for save/restore of callee-saved registers via libcalls

lewis-revill updated this revision to Diff 220677. lewis-revill added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Replace internal -mllvm option with target feature enabled through the clang frontend using -msave-restore. Repository: rG LLVM Github