[PATCH] D62565: [PowerPC] Exploiting to use mtvsrdd instruction when save called-saved GPR register to VSR registers

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2134
     unsigned Reg = CS.getReg();
-    if (!PPC::G8RCRegClass.contains(Reg) && !PPC::GPRCRegClass.contains(Reg)) {
+    // Simplify to do for G8RC only
+    if (!PPC::G8RCRegClass.contains(Reg)) {
----------------
The comment are comparing to old implementation? This is unnecessary?



================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2142
+    // into one VSR using the mtvsrdd instruction.
+    if (Subtarget.hasP9Vector() && LastSpilledVSR != 0) {
+      CS.setDstReg(LastSpilledVSR);
----------------
No need to check `hasP9Vector()` here, we already early exit above when `!Subtarget.hasP9Vector()` in line 2107.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2152
       CS.setDstReg(VolatileVFReg);
-      BVAllocatable.reset(VolatileVFReg);
+      if (Subtarget.hasP9Vector())
+        LastSpilledVSR = VolatileVFReg;
----------------
Similar to above, we only enable spill to VSR for P9, so no need to check here.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2185
+  // and their corresponding spilled GPRs.
+  for (unsigned i = 0, e = CSI.size(); i != e; ++i) {
+    if (!CSI[i].isSpilledToReg())
----------------
Can this be a `lamda` or function?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2190
+    unsigned Dst = CSI[i].getDstReg();
+    if (VSRToGPRs.find(Dst) == VSRToGPRs.end())
+      VSRToGPRs[Dst] = SmallVector<unsigned, 2>();
----------------
Please add comments about the mapping from GPR to VSR?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2256
+          BuildMI(MBB, MI, DL, TII.get(PPC::MTVSRDD), Dst)
+              .addReg(VSRToGPRs[Dst][0], getKillRegState(true))
+              .addReg(VSRToGPRs[Dst][1], getKillRegState(true));
----------------
Do we want to save the two GPRs in the order of endian order? or maybe fixed litten endian?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2266
+        } else {
+          assert(0);
+        }
----------------
Never use assert(0...), that's what `llvm_unreachable` is for.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2430
+          NumPEReloadVSR += 2;
+          BuildMI(MBB, I, DL, TII.get(PPC::MFVSRD), VSRToGPRs[Dst][0])
+              .addReg(TRI->getSubReg(Dst, PPC::sub_64));
----------------
Maybe the order here should be align with the restore order?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2440
+        } else {
+          assert(0);
+        }
----------------
Never use `assert(0)`, that's what `llvm_unreachable` is for.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.h:32
+  // Used to record the mapping of VSRs to spilled callee saved GPRs,
+  // where one VSR can be used to save several GPRs.
+  mutable std::map<unsigned, SmallVector<unsigned, 2>> VSRToGPRs;
----------------
`several` GPRs? 1 or 2?


================
Comment at: llvm/test/CodeGen/MIR/PowerPC/prolog_vec_spills.mir:16
     BLR8 implicit undef $lr8, implicit undef $rm
 
 # CHECK-LABEL: name:            test1BB
----------------
Please add comments about checking points.


================
Comment at: llvm/test/CodeGen/MIR/PowerPC/prolog_vec_spills.mir:23
+# CHECK-NEXT: $x14 = MFVSRD $vf0
+# CHECK-NEXT: $x15 = MFVSRLD killed $v0
+
----------------
It looks weird to have `x14` before `x15`, although they should be the same.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62565