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

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added inline comments.


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:401
+  default:
+    llvm_unreachable("Something has gone wrong!");
+  case /*s11*/ RISCV::X27: return "__riscv_save_12";
----------------
lenary wrote:
> Won't we hit this case if (after the hard float ABI lands) someone uses any of fs0-fs11? 
> 
> Would it not be better to return `nullptr`, so the code can bail out, rather than asserting?
> 
> The same applies in `getRestoreLibCallName`
I think the assertion is valid behaviour, since something has definitely gone wrong if any register not present in the list of callee-saved registers below is used as a callee-saved register. We would currently hit this if float ABI callee-saved registers are used, which would indicate that more logic needs to be added to explicitly handle that case.


================
Comment at: lib/Target/RISCV/RISCVMachineFunctionInfo.cpp:27
+  // function uses a varargs save area.
+  if (VarArgsSaveSize != 0)
+    return false;
----------------
MaskRay wrote:
> MaskRay wrote:
> > `return EnableSaveRestore && VarArgsSaveSize == 0;` ?
> Oh, why can't this stub just be inlined into getUseSaveRestoreLibCalls? It can be trivially computed every time its value is used. You can expand it later if this function ever becomes more complex.
So this `getUseSaveRestoreLibCalls` call gets called at multiple points in the pipeline. When I first implemented this function I wanted to be absolutely sure that at any stage in the pipeline the function returns the same result. Now that I think about it that would indicate incorrect behaviour anyway, so I'll make this change and see if I can find anything that breaks.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D62686