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

[Lewis Revill via Phabricator via cfe-commits]

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 save/restore.">;
----------------
lenary wrote:
> Given the clang option defaults to false, I think it should here too, to avoid confusion in other frontends.
This is simply a case of a confusing parameter of the SubtargetFeature class.

SubtargetFeature interprets this "true" string as 'Value the attribute to be set to by feature'. IE: when the feature is enabled, what value should the corresponding RISCVSubtarget variable be set to? Rather than a default value for that variable.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:278
   // Add CFI directives for callee-saved registers.
-  const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();
-  // Iterate over list of callee-saved registers and emit .cfi_restore
-  // directives.
-  for (const auto &Entry : CSI) {
-    Register Reg = Entry.getReg();
-    unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createRestore(
-        nullptr, RI->getDwarfRegNum(Reg, true)));
-    BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-        .addCFIIndex(CFIIndex);
+  if (!CSI.empty()) {
+    // Iterate over list of callee-saved registers and emit .cfi_restore
----------------
lenary wrote:
> Nit: You shouldn't need this `if` statement - the for loop just won't execute if CSI is empty, surely.
Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62686