[PATCH] D31679: Use PMADDWD to expand reduction in a loop

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

I suggest we leave the PMADDUBSW discussion for a separate patch.

Some minor comments inline.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34600
+
+  // SSSE3 has 8bit PMADDUBSW support, otherwise use 16bit PMADDWD
+  if (!Subtarget.hasSSSE3() && (Mode == MULS8 || Mode == MULU8))
----------------
Since this version always uses PMADDWD, we no longer care about Mode at all, and this entire if can go away.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34630
+  // Fill the rest of the output with 0
+  SDValue Zero = getZeroVector(Madd.getSimpleValueType(), Subtarget, DAG, DL);
+  SDValue Concat = DAG.getNode(ISD::CONCAT_VECTORS, DL, VT, Madd, Zero);
----------------
Isn't Madd.getSimpleValueType() always MVT::i32?



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34630
+  // Fill the rest of the output with 0
+  SDValue Zero = getZeroVector(Madd.getSimpleValueType(), Subtarget, DAG, DL);
+  SDValue Concat = DAG.getNode(ISD::CONCAT_VECTORS, DL, VT, Madd, Zero);
----------------
mkuper wrote:
> Isn't Madd.getSimpleValueType() always MVT::i32?
> 
Just a sanity check - this is correct irrespective of whether the initial value for the reduction phi is 0 or not, right?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34712
       return Sad;
+    if (SDValue Sad = combineMAddPattern(N, DAG, Subtarget))
+      return Sad;
----------------
Bikeshedding: maybe call this combineLoopMAddPattern() to match the other name?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:34712
       return Sad;
+    if (SDValue Sad = combineMAddPattern(N, DAG, Subtarget))
+      return Sad;
----------------
mkuper wrote:
> Bikeshedding: maybe call this combineLoopMAddPattern() to match the other name?
Sad -> Madd?


https://reviews.llvm.org/D31679