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

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

Michael/Adam,

Thanks very much for the comments! No problem about the delay - I'm happy to have the feedback. I've replied to your comments inline, and in one case asked a question about what we want the ValueMap interface change here to be.

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

> Yeah, hasStride does *not* seem to belong to WM.  We could have a wrapper of WM's getVectorValue in ILV that just swaps in the speculated stride value before calling WM::getVectorValue.
>
> getBroadcastInstrs on the other hand probably does belong to WM because it's part of the whole picture how values are mapped.  So probably making it stand-alone utility is the way to go.
>
> Anyhow this all is certainly for another patch.  I just wanted to see what you guys thought about moving even more of the mapping responsibility out of ILV to WM.


Sorry to miss replying to these comments earlier. Yes, I think it makes sense to move the mapping related tasks out of ILV and into ValueMap. I think getBroadcastIstrs, since it's currently used by both ValueMap and ILV, could actually live in VectorUtils. I think it just needs to know where to insert the code.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:545-553
@@ -509,4 +544,11 @@
+
+    /// \return A reference to a new vector map entry corresponding to \p Key.
+    /// The key should not already be in the map. If \p Val is provided, each
+    /// vector value is initialized to this value.
+    VectorParts &initVector(Value *Key, Value *Val = nullptr) {
+      assert(!hasVector(Key) && "VectorParts already initialized");
+      auto &Entry = VectorMapStorage[Key];
       Entry.assign(UF, Val);
       return Entry;
     }
 
----------------
anemet wrote:
> If we can't change all users to use init*, it would be better to keep both functionalities separate rather than hiding it behind a default parameter.  Just keep the ugly get interface as well, so that the users are easy to find.  Hopefully we can migrate all users to init* in the future.
I actually did change all users to either init(Vector|Scalar) or get(Vector|Scalar)Value. The default parameter here was added as a replacement for the "splat" interface, which I removed. I can add this back though.

But looking over your other comments, I think you're wondering why getVectorValue returns a reference to the map entries that are then able to be overridden and set by the caller? This is basically the same as the original WidenMap.get I agree that's, strange.

So to be clear, you're suggesting that for now we keep the original "get" and "splat" interfaces along side the new init* interfaces? Or are you suggesting that we abandon the init* interfaces for now as well?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:558
@@ +557,3 @@
+    /// scalar value is initialized to this value.
+    ScalarParts &initScalar(Value *Key, Value *Val = nullptr) {
+      assert(!hasScalar(Key) && "ScalarParts already initialized");
----------------
mkuper wrote:
> Do we need the Val parameter? It doesn't look like we call this with a non-default Val anywhere.
No, I'll remove it. Thanks!

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2322
@@ +2321,3 @@
+    // If V doesn't produce a value, just return the initialized entry.
+    if (V->getType()->isVoidTy())
+      return Entry;
----------------
mkuper wrote:
> This looks a bit odd.
> 
> When does this happen? I don't think we call getVectorValue() for stores - and if we do, I'm not sure what the expected result is.
> If it doesn't happen, perhaps this ought to be an assert?
I agree, I don't think this should happen. This was taken from the scalarization code, and I kept it this way to make the patch as NFC as possible. The only possible, but unlikely, issue I can think of is if we use presence in WidenMap as an indicator for "already vectorized". But again, I don't think we do this. I think it's perfectly sensible to add an assert. I'll update the patch.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2368
@@ +2367,3 @@
+  auto *U = getVectorValue(V)[Part];
+  if (!U->getType()->isVectorTy())
+    return U;
----------------
mkuper wrote:
> I'd still appreciate a comment (and maybe an assert) that this is for VF==1.
Sure, sounds good!

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2371
@@ +2370,3 @@
+
+  // Otherwise, the value from the original loop has been vectorized and is
+  // represented by UF vector values. Extract and return the requested scalar
----------------
mkuper wrote:
> The comment is a bit odd, because the "Otherwise" looks like it refers to the previous comment ("If the value has not been scalarized...") while in fact it refers to the one before ("If the value from the original loop has not been vectorized").
The comments are confusing; I'll fix that. The intent was the following. Case 2: "value has been scalarized." Case 3: "value hasn't been scalarized, but also hasn't been vectorized (VF=1)". Case 4: "value has been vectorized".

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2587-2589
@@ -2471,5 +2586,5 @@
 
-        VectorParts &Entry = WidenMap.get(Member);
+        VectorParts &Entry = getVectorValue(Member);
         Entry[Part] =
             Group->isReverse() ? reverseVector(StridedVec) : StridedVec;
       }
----------------
anemet wrote:
> This is weird in terms of its interface use.  getVectorValue computes values and then this one overrides it?!  I think we should just stick with the &get interface here and clean in it up  in the future.
Sure. To be clear, getVectorValue returns a reference to an entry (which can be overridden) if it exists in the map, and only computes values if it's not already there.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2679
@@ -2565,4 +2678,3 @@
 
-  Constant *Zero = Builder.getInt32(0);
-  VectorParts &Entry = WidenMap.get(Instr);
+  VectorParts &Entry = getVectorValue(Instr);
   VectorParts VectorGep;
----------------
anemet wrote:
> Is this call necessary?
Yes, the Entry values are set later on in the function. (The diff here is a bit misleading. Here, I had replaced WidenMap.get with getVectorValue).

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2859-2862
@@ -2786,7 +2858,6 @@
 
-  Value *UndefVec =
-      IsVoidRetTy ? nullptr
-                  : UndefValue::get(VectorType::get(Instr->getType(), VF));
-  // Create a new entry in the WidenMap and initialize it to Undef or Null.
-  VectorParts &VecResults = WidenMap.splat(Instr, UndefVec);
+  // The instruction will not be vectorized. Erase its vector entry from
+  // VectorLoopValueMap and get a new scalar entry instead.
+  VectorLoopValueMap.eraseVector(Instr);
+  auto &Entry = VectorLoopValueMap.initScalar(Instr);
 
----------------
anemet wrote:
> It would be good to explain the scenario under which we have already created a vector value for this value.
I think Michael covered this in his comment. I'll update the comment here to explain why the erase is needed.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2861
@@ +2860,3 @@
+  // VectorLoopValueMap and get a new scalar entry instead.
+  VectorLoopValueMap.eraseVector(Instr);
+  auto &Entry = VectorLoopValueMap.initScalar(Instr);
----------------
mkuper wrote:
> Just to make sure I'm not missing anything - the reason this is needed is because we eagerly created the vector entry in the beginning of vectorizeBlockInLoop(), right? We don't *actually* have a vector entry at this point, it's just an empty placeholder - but we need to delete it, so that if we need a real vector entry, it will get constructed from scratch from the scalars?
That's right! I'll update the comment to better clarify this.


https://reviews.llvm.org/D23169