[PATCH] D12325: Improve ISel using across lane addition reduction

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Thanks for working on this, Jun.  Mostly style nits with a few other comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8583
@@ +8582,3 @@
+  SDValue N1 = N->getOperand(1);
+  if (N0->getOpcode() != ISD::ADD || !isa<ConstantSDNode>(N1) ||
+      cast<ConstantSDNode>(N1)->getZExtValue())
----------------
To improve readability, you could expand this into two parts:

  // If the extract isn't feeding an ADD, can't do such combine.
  if (N0->getOpcode() != ISD::ADD)
    return SDValue();

  // Vector extract idx must constant zero. 
  if (!isa<ConstantSDNode>(N1) ||  cast<ConstantSDNode>(N1)->getZExtValue())
    return SValue();

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8595
@@ +8594,3 @@
+  // Iterate over each step of the reduction.
+  while (NumAddElt <= NumVecElts / 2) {
+    if (InputADD.getOpcode() != ISD::ADD)
----------------
The computation of NumVecElts / 2 appears to be loop invariant.  Mind hoisting it out of the conditional check for clarity?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8609
@@ +8608,3 @@
+    //   %add = add %1, %0
+    //   %svn= vector_shuffle %add, <2, 3, u, u>
+    //   %inputadd = add %add, %svn
----------------
Please add a space between %svn and =.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8611
@@ +8610,3 @@
+    //   %inputadd = add %add, %svn
+    if (SV.getOperand(0) != ADD || !SV.hasOneUse())
+      return SDValue();
----------------
I don't know the correct answer, but is the SV.hasOneUse() check necessary?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8622
@@ +8621,3 @@
+    //   step 1 : <1,u,u,u,u,u,u,u>
+    for (unsigned int i = 0; i < NumVecElts; i++)
+      if ((i >= NumAddElt && Mask[i] >= 0) ||
----------------
Please pre-increment i.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8630
@@ +8629,3 @@
+  }
+  SDLoc dl(N);
+  return DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, N->getValueType(0),
----------------
Please use DL rather than dl.


http://reviews.llvm.org/D12325