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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

LGTM. The remaining comments are stylistic nits that can be addressed on the commit and do not require another round of review. Thank you for your patience with this review.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3215
+  // Try to get the first register.
+  unsigned Reg = State.AllocateReg(HiRegList, LoRegList);
+  if (!Reg)
----------------
I don't really understand why this is handled differently than the parameters in terms of how the allocation is done. It is perhaps a good indicator that a comment explaining the difference would be useful.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3578
+      // virtual ones.
+      if (VA.getLocVT() == MVT::f64 && Subtarget.hasSPE()) {
+        assert(i + 1 < e && "No second half of double precision argument");
----------------
I think it would be good to keep using the Hi/Lo naming here as well and make it very obvious that you're adding two live-in registers and adding copies from them into virtual regs. Something along the lines of:
```
if (VA.getLocVT() == MVT::f64 && Subtarget.hasSPE()) {
  assert(i + 1 < e && "No second half of double precision argument");
  unsigned RegLo = MF.addLiveIn(VA.getLocReg(), RC);
  unsigned RegHi = MF.addLiveIn(ArgLocs[++i].getLocReg(), RC);
  SDValue ArgValueLo = DAG.getCopyFromReg(Chain, dl, RegLo, MVT::i32);
  SDValue ArgValueHi = DAG.getCopyFromReg(Chain, dl, RegHi, MVT::i32);

  if (!Subtarget.isLittleEndian())
    std::swap (ArgValueLo, ArgValueHi);
  ArgValue = DAG.getNode(PPCISD::BUILD_SPE64, dl, MVT::f64, ArgValueLo, 
                         ArgValueHi);
} else {
  // existing code ...
}
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:5545
   // Walk the register/memloc assignments, inserting copies/loads.
-  for (unsigned i = 0, j = 0, e = ArgLocs.size();
+  for (unsigned i = 0, realArgIdx = 0, j = 0, e = ArgLocs.size();
        i != e;
----------------
I think it would be nice to add a comment explaining what the induction variables track (correct the below if it's wrong):
```
i - Tracks the index into the list of registers allocated for the call
RealArgIdx - Tracks the index into the list of actual function arguments
j - Tracks the index into the list of byval arguments
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:5600
+      if (Subtarget.hasSPE() && Arg.getValueType() == MVT::f64) {
+        bool isLittleEndian = Subtarget.isLittleEndian();
+        SDValue SVal =
----------------
Minor nit: this doesn't really match the naming convention. Perhaps `IsLE`?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:6756
   // Copy the result values into the output registers.
-  for (unsigned i = 0; i != RVLocs.size(); ++i) {
+  for (unsigned i = 0, realResIdx = 0; i != RVLocs.size(); ++i, ++realResIdx) {
     CCValAssign &VA = RVLocs[i];
----------------
Similar comment for this code as above regarding induction variables and the naming of `isLittleEndian`.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.h:1114
     SDValue LowerBITCAST(SDValue Op, SelectionDAG &DAG) const;
+    SDValue LowerEXTRACT_ELEMENT(SDValue Op, SelectionDAG &DAG) const;
 
----------------
This does not appear to be used. Perhaps an artifact remaining from a previous revision?


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:236
+                           SDTypeProfile<1, 2,
+                             [SDTCisFP<0>, SDTCisSameAs<1,2>]>,
+                           []>;
----------------
I think all the types have to be exact - `i32` inputs, `f64` output. Might as well make that explicit: `[SDTCisVT<0, f64>, SDTCisVT<1, i32>, SDTCisVT<2, i32>]`

Similarly below.


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

https://reviews.llvm.org/D54583