[PATCH] D39386: [Power9] Allow gpr callee saved spills in prologue to vector registers rather than stack

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Thank you for updating the code to get the CSR list from `RegInfo`. It looks much cleaner than having a static list of registers. I think the code can be cleaned up a bit further still, but overall this looks great.



================
Comment at: llvm/include/llvm/CodeGen/MachineFrameInfo.h:64
+  unsigned getDstReg()                     const { return DstReg; }
   void setFrameIdx(int FI)                       { FrameIdx = FI; }
+  void setDstReg(unsigned SpillReg)              { DstReg = SpillReg; }
----------------
I think `setFrameIdx()` and `setDstReg()` should probably update `SpilledToReg`.


================
Comment at: llvm/include/llvm/CodeGen/MachineFrameInfo.h:265
+  /// register.  Beyond its use by the prolog/ epilog code inserter,
+  /// this data used for debug info and exception handling.
   std::vector<CalleeSavedInfo> CSInfo;
----------------
... this data *is* used...


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1963
+    return false;
+
+  BitVector BVAllocatable = TRI->getAllocatableSet(MF);
----------------
A comment here such as:
`// Build a BitVector of VSRs that can be used for spilling GPRs.`


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1971
+
+  // Set to 0 if the register is not a volatile VF/F8/F4 register.
+  for (unsigned Reg = 0; Reg < BVAllocatable.size(); Reg++) {
----------------
Why the 3 register classes? Don't we only want to keep VF/F8 registers in this BitVector?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1976
+                                !PPC::F4RCRegClass.contains(Reg) &&
+                                !PPC::VFRCRegClass.contains(Reg))))
+      BVAllocatable.reset(Reg);
----------------
Can you also clear any of the registers that satisfy `MF.getRegInfo().isPhysRegUsed(VolatileVFReg)` so you:
- Provide early exit if there are no unused caller-saved VSRs
- Simplify the spilling code below (so you don't need to loop and check this there)


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1980
+
+  if (BVAllocatable.none())
+    return false;
----------------
If you do the above, you can sink this into the loop so we exit as soon as we have used up all the usable VSRs.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1990
+    }
+    for (unsigned VolatileVFReg = BVAllocatable.find_first();
+         VolatileVFReg < BVAllocatable.size();
----------------
If you do the above, this whole thing just becomes a call to `find_first()`.


https://reviews.llvm.org/D39386