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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2259
   bool AllSpilledToReg = true;
+  unsigned LastSpilledVSR = 0;
   for (auto &CS : CSI) {
----------------
This name is strange. We are not spilling VSR's here. Perhaps `LastVSRUsedForSpill`?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2310
+    if (Info.isSpilledToReg()) {
+      VSRToGPRs.FindAndConstruct(Info.getDstReg())
+          .second.push_back(Info.getReg());
----------------
I think an assert to ensure we are not trying to put more than two GPRs in a VSR should be added here.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2369
+        if (VSRToGPRs[Dst].size() == 2) {
+          assert(Subtarget.hasP9Vector());
+
----------------
Please avoid asserts without text explaining the assert.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.h:33
+  // TODO: Use local table in methods to avoid this mutable member.
+  mutable DenseMap<unsigned, SmallVector<Register, 2>> VSRToGPRs;
+
----------------
I am curious why this maps from `unsigned`. Why not from `Register`?
Also, I think it makes sense to map to `std::pair` since there will be (at most) two GPR's.

Finally, I think the name is confusing. I understand that it is mapping a VSR to GPRs, but since this optimization is moving stuff between registers, it can easily be misunderstood to mean that we are moving a VSR to GPRs. I think a better name would be something like `VSRContainsGPRs`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62565