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

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

Hi Jongwon,

I've inlined a few comments. And I agree with Michael about the compile-time experiments. It would be helpful to see what kind of impact trying different register sizes will have. Thanks!


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3501
@@ -3500,1 +3500,3 @@
   bool tryToVectorizeList(ArrayRef<Value *> VL, BoUpSLP &R,
+                          unsigned VecRegSize = 128,
+                          bool vectorizeStoreChain = false,
----------------
Unless I missed something, it looks to me like every use of tryToVectorizeList passes VecRegSize. Why make the parameter optional? 

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4222
@@ -4264,3 +4221,3 @@
                                         BoUpSLP &R, TargetTransformInfo *TTI,
                                         unsigned MinRegSize) {
   if (!ShouldVectorizeHor)
----------------
I think it would be less confusing and more consistent if MinRegSize was renamed to VecRegSize here.


http://reviews.llvm.org/D18237