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

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:655
@@ +654,3 @@
+             "ScalarParts has wrong dimensions");
+      ScalarMapStorage[Key] = std::move(Entry);
+      return ScalarMapStorage[Key];
----------------
anemet wrote:
> 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).
Ah, you're right. Thanks for pointing this out. I'll update this to be by value.

================
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");
----------------
anemet wrote:
> Please add a comment discouraging the use of this function in favor of initVector.
Sure.

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

================
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();
----------------
anemet wrote:
> I am not a big fan of auto in cases where the type is not obvious.  At least we should have const.  Same later.
Sure, instead of auto, I'll just make these "const VectorParts &" to make things clear.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4389
@@ -4297,3 +4388,3 @@
   for (Instruction &I : *BB) {
-    VectorParts &Entry = WidenMap.get(&I);
+    VectorParts Entry(UF);
 
----------------
anemet wrote:
> 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.
I agree. There's a few cases that don't actually use Entry. I'll update the patch.


https://reviews.llvm.org/D23169