[PATCH] [SLPVectorization] Vectorize flat addition in a single tree (+(+(+ v1 v2) v3) v4)

[Charlie Turner]

Hey Suyog,

Looks interesting :-) I can't speak for the technical content, but I did notice a few stylistic problems which might save you some patch ping-pong later.

See inline.

Cheers,
Charlie.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3327-3328
@@ -3317,1 +3326,4 @@
 
+  /// Try to find a flat horizontal reduction. The tree structure of such
+  /// addition of type a[0]+a[1]+a[2]+a[3].... will be be like
+  /// (+(+(+ a[0], a[1]), a[2]), a[3])....
----------------
Why three slashes? Normally we use two.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3352-3353
@@ +3351,4 @@
+        SmallVector<BinaryOperator*, 32> Stack;
+        Value* Op0 = B->getOperand(0);
+        Value* Op1 = B->getOperand(1);
+
----------------
It's more conventional in LLVM to type these as `Value *Op0`, the style you're using in this function varies from declaration to declaration. There are several more instances of this in the patch.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3359-3360
@@ +3358,4 @@
+        // None of the operands are binary.
+        if(!Op0Bin && !Op1Bin)
+            return false;
+
----------------
Add a space after the `if`. There are several instances of this in the patch, including for other control constructs.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3606-3607
@@ +3605,4 @@
+    // %5 = add <4 x> %3, %4
+    // %6 = extractelement %5 <0>
+        if(IsHAdd) {
+            unsigned VecElem = VecTy->getVectorNumElements();
----------------
Looks like you're an indent level too far in here.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3610-3613
@@ +3609,6 @@
+            unsigned NumRedxLevel = Log2_32(VecElem);
+            HAddCost = NumRedxLevel * (TTI->getArithmeticInstrCost(ReductionOpcode,VecTy)
+                       + TTI->getShuffleCost(TargetTransformInfo::SK_ExtractSubvector,
+                                                 VecTy, VecElem/2, VecTy))
+                    + TTI->getVectorInstrCost(Instruction::ExtractElement, VecTy, 0);
+        }
----------------
This looks a bit weird, I suggest you run it through `clang-format`.

http://reviews.llvm.org/D6818

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/