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

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

In https://reviews.llvm.org/D23169#506561, @anemet wrote:

> As we're piling on more complexity on the top of WidenMap, I feel like that the completely permissive ValueParts &get() interface makes it really hard to reason about this code.  The new eraseVector API is certainly a warning sign.


Right, that is confusing. We use "get" (now getScalar/getVector) to return an existing entry in the map or initialize an empty entry it if it's not already there. We use "getVectorValue" to return an existing entry or construct a non-empty entry (by broadcasting or, now, by inserting scalar elements into a vector). getVector/getScalar should probably just be initializers. For example, I think we should be able to replace all but one instance of getVector with getVectorValue. Then, getVector/getScalar could be renamed to initVector/initScalar, which would assert or noop if we try to initialize a key more than once. What do you think?

> I think the new logic is correct because we first widen definitions before getting to the uses.  So when widening the definition we either create the vector variant or the scalar variant or in some cases of int induction variables both.  And then as we encounter uses, we derive the missing scalar or vector variants from each unless we already have a value for it.  Please correct me if if I am misinterpreting this.  It would also be good to have a high-level overview of these interactions commented somewhere before the class.


That's right. Sure I'll add some high-level comments.

> Anyhow, this all seems like the property of WidenMap, so I am wondering how hard it would be to pull getScalarValue and getVectorValue under that class.  (I am also not sure that hiding the class inside InnerLoopVectorizer in a .cpp file is really adding anything but certainly hurts readability as the class is growing.)


I think moving getVectorValue and getScalarValue into WidenMap might be tricky. The issue is that getVectorValue uses getBroadcastInstrs, which is a member of InnerLoopVectorizer, and hasStride from LoopVectorizationLegality. For getBroadcastInstrs, would we pass an InnerLoopVectorizer* to WidenMap? Alternatively, we could probably make getBroadcastInstrs into a utility (maybe in VectorUtils?). What about hasStride? What do you think?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:549
@@ +548,3 @@
+    std::map<Value *, VectorParts> VectorMapStorage;
+    std::map<Value *, VectorParts> ScalarMapStorage;
+
----------------
mkuper wrote:
> 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.
Sounds good to me.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:630
@@ -605,1 +629,3 @@
+  /// vectorized and scalarized.
+  ValueMap WidenMap;
 
----------------
mkuper wrote:
> Bike-shedding - maybe change the name of the variable? "WidenMap" made sense when it mapped scalar values to the corresponding "wide" vector values.
I actually thought about doing this, but couldn't really come up with anything better. Do you have any ideas? What about VectorLoopMap?

================
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);
----------------
mkuper wrote:
> I -> Lane ?
Makes sense.

================
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)
----------------
mkuper wrote:
> For clarity, I'd prefer a temp value here, and an assignment to Parts[Part] in the end.
Sure.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2343
@@ +2342,3 @@
+  if (!U->getType()->isVectorTy())
+    return U;
+
----------------
mkuper wrote:
> Is this for VF==1, for uniform values, or both?
Nice question! This is for the VF == 1 case, and it prevents us from trying to extract an element from a non-vector type. I moved this check from the existing scalarization code (line 2827). But we could replace the if condition here with VF == 1 if that makes more sense.

To be clear, if a value is not an instruction in the loop, it's handled by the first check. For the instructions in Uniforms, I believe we will vectorize/scalarize them, and then the unused pieces will be later deleted.


https://reviews.llvm.org/D23169