[PATCH] D18237: [SLPVectorizer] Change MinVecRegSize from 128 bits to 64 bits

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3427
@@ +3426,3 @@
+    // <2 x 32-bit data type>, <4 x 16-bit data type>, <8 x 8-bit data type>.
+    llvm::Triple TargetTriple(F.getParent()->getTargetTriple());
+    bool IsAArch64 = TargetTriple.getArch() == llvm::Triple::aarch64 ||
----------------
I'm thinking this should be a TTI hook, so each target can define the MinVecRegSize.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3723
@@ +3722,3 @@
+  bool SuccessToVectorizeList = false;
+  for (unsigned Size = MaxVecRegSize; Size >= MinVecRegSize; Size /= 2) {
+    if (tryToVectorizeList(VL, R, None, true, Size)) {
----------------
Maybe use VecRegSize, rather than Size here?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3747
@@ -3727,3 +3746,3 @@
 
   // FIXME: Register size should be a parameter to this function, so we can
   // try different vectorization factors.
----------------
I believe you've addressed this fix me, correct?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3987
@@ -3966,3 +3986,3 @@
     ReducedValueOpcode = 0;
     // FIXME: Register size should be a parameter to this function, so we can
     // try different vectorization factors.
----------------
Same. Remove FIXME.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4342
@@ +4341,3 @@
+      bool SuccessToVectorizeList = false;
+      for (unsigned Size = MaxVecRegSize; Size >= MinVecRegSize; Size /= 2) {
+        if (tryToVectorizeList(makeArrayRef(IncIt, NumElts), R, None, false,
----------------
Maybe use VecRegSize, rather than Size here?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4419
@@ -4383,4 +4418,3 @@
                 dyn_cast<BinaryOperator>(SI->getValueOperand())) {
-          if (canMatchHorizontalReduction(nullptr, BinOp, R, TTI,
-                                          MinVecRegSize) ||
-              tryToVectorize(BinOp, R)) {
+          // if (canMatchHorizontalReduction(nullptr, BinOp, R, TTI,
+          //                                 MinVecRegSize) ||
----------------
I assume this should be deleted, rather than commented out.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4489
@@ -4444,2 +4488,3 @@
       // extraction.
-      if (tryToVectorizeList(BuildVectorOpds, R, BuildVector)) {
+      bool SuccessToVectorizeList = false;
+      for (unsigned Size = MaxVecRegSize; Size >= MinVecRegSize; Size /= 2) {
----------------
I don't think you need the temporary variable.  

  for () {
    if (tryToVectorizeList()) {
      Changed = true;
      it = BB->begin();
      e = BB->end();
      break;
    }
  }

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4586
@@ -4534,2 +4585,3 @@
       // full-blown top-down phase beginning at the consecutive loads.
-      Changed |= tryToVectorizeList(Bundle, R);
+      // Changed |= tryToVectorizeList(Bundle, R);
+      bool SuccessToVectorizeList = false;
----------------
Please remove.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4587
@@ +4586,3 @@
+      // Changed |= tryToVectorizeList(Bundle, R);
+      bool SuccessToVectorizeList = false;
+      for (unsigned Size = MaxVecRegSize; Size >= MinVecRegSize; Size /= 2) {
----------------
I don't think you need this temporary value.  You can just do something like the below, correct?

  for (...)
    if (tryToVectorizeList()) {
      Changed = true;
      break;
    }


http://reviews.llvm.org/D18237