[PATCH] D23169: [LV] Unify vector and scalar maps

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

+1000 on the design and the cleanup, some nits about the implementation.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:549
@@ +548,3 @@
+    std::map<Value *, VectorParts> VectorMapStorage;
+    std::map<Value *, VectorParts> ScalarMapStorage;
+
----------------
Perhaps use a 2D vector for this ("ScalarParts"), instead of the 1D VectorParts?
I don't think we actually gain anything from using VectorParts for both - even in the one place where we do move a VectorParts from one map to the other (line 2300), you don't actually break the abstraction.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:630
@@ -605,1 +629,3 @@
+  /// vectorized and scalarized.
+  ValueMap WidenMap;
 
----------------
Bike-shedding - maybe change the name of the variable? "WidenMap" made sense when it mapped scalar values to the corresponding "wide" vector values.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2175
@@ -2147,3 +2174,3 @@
   for (unsigned Part = 0; Part < UF; ++Part)
     for (unsigned I = 0; I < VF; ++I) {
       auto *StartIdx = ConstantInt::get(ScalarIVTy, VF * Part + I);
----------------
I -> Lane ?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2310
@@ +2309,3 @@
+    for (unsigned Part = 0; Part < UF; ++Part) {
+      Parts[Part] = UndefValue::get(VectorType::get(V->getType(), VF));
+      for (unsigned Width = 0; Width < VF; ++Width)
----------------
For clarity, I'd prefer a temp value here, and an assignment to Parts[Part] in the end.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2343
@@ +2342,3 @@
+  if (!U->getType()->isVectorTy())
+    return U;
+
----------------
Is this for VF==1, for uniform values, or both?


https://reviews.llvm.org/D23169