[PATCH] D54583: PowerPC: Optimize SPE double parameter calling setup

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jdoerfert.

Also, as we discussed on IRC, please provide full context to make it easier to review.
I am actually OK with this revision as long as the comments are addressed, but I'm just requesting another revision to ensure all the comments have been addressed.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3164
+
+  unsigned i;
+  for (i = 0; i < sizeof(HiRegList) / sizeof(HiRegList[0]); ++i)
----------------
Since this isn't local to a very small loop, I think a more descriptive name is in order. Perhaps `RegIdx`?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3165
+  unsigned i;
+  for (i = 0; i < sizeof(HiRegList) / sizeof(HiRegList[0]); ++i)
+    if (HiRegList[i] == Reg)
----------------
I think it is more obvious to use `sizeof(MCPhysReg)` rather than `sizeof(HiRegList[0])`.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3196
+      break;
+
+  State.addLoc(CCValAssign::getCustomReg(ValNo, ValVT, Reg, LocVT, LocInfo));
----------------
I can't say I am overly familiar with this code, but we don't need to call `State.AllocateReg()` for the low register?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3550
+      if (VA.getLocVT() == MVT::f64 && Subtarget.hasSPE()) {
+        // Transform the arguments stored in physical registers into
+        // virtual ones.
----------------
I don't think it's useful to duplicate a comment down both paths in a condition.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3555
+
+        SDValue ArgValue2;
+        Reg = MF.addLiveIn(ArgLocs[++i].getLocReg(), RC);
----------------
Perhaps something like
```
assert(i + 1 < e &&
       "No second half of double precision argument");
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:5520
   // Walk the register/memloc assignments, inserting copies/loads.
-  for (unsigned i = 0, j = 0, e = ArgLocs.size();
+  for (unsigned i = 0, realI = 0, j = 0, e = ArgLocs.size();
        i != e;
----------------
`i, realI, j` are not adequate given this rather complex iteration space... Please name them for what they are meant to refer to.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:6755
+          DAG.getNode(PPCISD::EXTRACT_SPE, dl, MVT::i32, Arg,
+                      DAG.getIntPtrConstant(isLittleEndian ? 0 : 1, dl));
+      Chain = DAG.getCopyToReg(Chain, dl, VA.getLocReg(), SVal, Flag);
----------------
Can we be consistent about how we specify the index to the extract between here and above? I am OK with either approach as long as we use the same.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:237
+                             [SDTCisFP<0>, SDTCisSameSizeAs<1,2>,
+                              SDTCisSameAs<1,2>]>,
+                           []>;
----------------
Seems like we don't need to separately state that the types are the same and that they're the same size?


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

https://reviews.llvm.org/D54583