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

[Luís Marques via Phabricator via cfe-commits]

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 should be addressed.

@apazos Have you considered tweaking the patch code to not do a tail call, just to check if that's what's causing the remaining failures? I'm not sure if that's too hard, but it could eventually be easier than drilling into the failing cases.



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:27
+// registers.
+static int getLibCallID(const MachineFunction &MF,
+                        const std::vector<CalleeSavedInfo> &CSI) {
----------------
The return value isn't used as just an opaque index, it also reflects the frame size and is used for that purpose. The function comment should probably reflect that.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:34
+
+  unsigned MaxReg = 0;
+  for (auto &CS : CSI)
----------------
Use `Register` and `RISCV::NoRegister`. (You'll have to use `MaxReg.id()` instead in the call to `max`).


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:36
+  for (auto &CS : CSI)
+    if (CS.getFrameIdx() < 0)
+      MaxReg = std::max(MaxReg, CS.getReg());
----------------
Might be worth adding a small comment explaining how this serves as a filters for the registers we are interested in. Or point to a later relevant comment?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:39
+
+  if (MaxReg == 0)
+    return -1;
----------------
Ditto `NoRegister`.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:66
+                    const std::vector<CalleeSavedInfo> &CSI) {
+  static const char *const spillLibCalls[] = {
+    "__riscv_save_0",
----------------
Check LLVM naming convention capitalization. Ditto other vars here.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:93
+                      const std::vector<CalleeSavedInfo> &CSI) {
+  static const char *const restoreLibCalls[] = {
+    "__riscv_restore_0",
----------------
Check LLVM naming convention capitalization. Ditto other vars here.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:190
 
+static std::vector<CalleeSavedInfo>
+getNonLibcallCSI(const std::vector<CalleeSavedInfo> &CSI) {
----------------
This could probably use `SmallVector`.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:706
+  for (auto &CS : reverse(NonLibcallCSI)) {
+    unsigned Reg = CS.getReg();
+    const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg);
----------------
Ditto `Register`.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:95
+// by save/restore libcalls.
+static const std::map<unsigned, int> FixedCSRFIMap = {
+  {/*ra*/  RISCV::X1,   -1},
----------------
Use `IndexedMap` instead?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62686