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

[Adam Nemet via llvm-commits]

anemet added a comment.

In https://reviews.llvm.org/D23169#519875, @mssimpso wrote:

> Also, because we no longer eagerly create empty mappings, eraseVector is no longer needed.


Hurray!

This is all looking pretty nice now!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:655
@@ +654,3 @@
+             "ScalarParts has wrong dimensions");
+      ScalarMapStorage[Key] = std::move(Entry);
+      return ScalarMapStorage[Key];
----------------
I don't think this is what you want.  This will still do a copy because Entry is a const &.  I think that this function should take Entry by value (or by r-value but that would make it harder to use).

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:659-661
@@ -597,1 +658,5 @@
 
+    /// \return A reference to the vector map entry corresponding to \p Key.
+    /// The key should already be in the map.
+    VectorParts &getVector(Value *Key) {
+      assert(hasVector(Key) && "Vector entry not initialized");
----------------
Please add a comment discouraging the use of this function in favor of initVector.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2309
@@ -2232,1 +2308,3 @@
+  }
+  VectorLoopValueMap.initScalar(EntryVal, Entry);
 }
----------------
This should std::move Entry.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3830
@@ -3738,3 +3829,3 @@
     // This is the vector-clone of the value that leaves the loop.
-    VectorParts &VectorExit = getVectorValue(LoopExitInst);
+    auto &VectorExit = getVectorValue(LoopExitInst);
     Type *VecTy = VectorExit[0]->getType();
----------------
I am not a big fan of auto in cases where the type is not obvious.  At least we should have const.  Same later.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4389
@@ -4297,3 +4388,3 @@
   for (Instruction &I : *BB) {
-    VectorParts &Entry = WidenMap.get(&I);
+    VectorParts Entry(UF);
 
----------------
Would it be safer to do this inside the blocks?  I want to make sure you didn't leave a Entry[] = ... without a corresponding initVector.


https://reviews.llvm.org/D23169