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

[Adam Nemet via llvm-commits]

anemet added a comment.

Matt, sorry about the delay.


================
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;
     }
 
----------------
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.

================
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;
       }
----------------
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.

================
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;
----------------
Is this call necessary?

================
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);
 
----------------
It would be good to explain the scenario under which we have already created a vector value for this value.


https://reviews.llvm.org/D23169