[PATCH] D24796: [SLPVectorizer] Fix for PR25748: reduction vectorization after loop unrolling.

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1797
@@ +1796,3 @@
+  // We only handle trees of heights 1 and 2.
+  if (VectorizableTree.size() == 1 && !VectorizableTree[0].NeedToGather)
+    return true;
----------------
Why do you need this change?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4171
@@ -4167,3 +4170,3 @@
       Value *NextV = TreeN->getOperand(EdgeToVist);
       // We currently only allow BinaryOperator's and SelectInst's as reduction
       // values in our tree.
----------------
This comment looks wrong now. Could you replace it with a comment that explains the current situation?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4586
@@ -4573,3 +4585,3 @@
           DEBUG(dbgs() << "SLP: Found a return to vectorize.\n");
-          if (tryToVectorizePair(BinOp->getOperand(0),
-                                 BinOp->getOperand(1), R)) {
+          if (canMatchHorizontalReduction(nullptr, BinOp, R, TTI,
+                                          R.getMinVecRegSize()) ||
----------------
I actually started coding a similar patch at some point, but decided against it, because the "return" here seems really fairly accidental.
You could just as easily have something like:


```
int test(unsigned int *p) {
  int sum = 0;
  for (int i = 0; i < 8; i++)
    sum += p[i];
  return sum + 5;
}
```

It may be the case that having it feed a return is an important special case, but maybe we can work out a full solution for this.


https://reviews.llvm.org/D24796