[PATCH] D18237: [SLPVectorizer] Try to vectorize in the range from MaxVecRegSize to MinVecRegSize

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Jongwon,

Thanks for the work, please find some comments below.

Michael


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3687
@@ -3732,9 +3686,3 @@
 
-  for (Value *V : VL) {
-    Type *Ty = V->getType();
-    if (!isValidElementType(Ty))
-      return false;
-    Instruction *Inst = dyn_cast<Instruction>(V);
-    if (!Inst || Inst->getOpcode() != Opcode0)
-      return false;
-  }
+  if (!vectorizeStoreChain) {
+    for (Value *V : VL) {
----------------
Nitpick: I'd rather swap if and else blocks to avoid negation in the condition.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3696-3697
@@ -3741,1 +3695,4 @@
+    }
+  } else if (!isPowerOf2_32(Sz) || VF < 2)
+    return false;
 
----------------
I think this check should be combined with the one below:
```
if (!isPowerOf2_32(OpsWidth) || OpsWidth < 2)
  break;
```
and it should be done independently on `vectorizeStoreChain` flag.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4544
@@ -4557,4 +4543,3 @@
       unsigned Len = std::min<unsigned>(CE - CI, 16);
-      Changed |= vectorizeStores(makeArrayRef(&it->second[CI], Len),
-                                 -SLPCostThreshold, R);
+      Changed |= vectorizeStores(makeArrayRef(&it->second[CI], Len), R);
     }
----------------
`SLPCostThreshold` disappeared after this change. Was it intentional?


http://reviews.llvm.org/D18237