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

[Adam Nemet via llvm-commits]

anemet added inline comments.

================
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;
     }
 
----------------
mssimpso wrote:
> 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?
> 
> 
Yes, the former.  The init stuff is great, my hope is that it will slowly become the main interface along with get{Vector,Scalar}Value.  I think we want to use 'get' when all we do is allocate the entries.  'init' when we actually set the value and get*Value when we let the variants be derived from each other.

I think I still don't understand initVector with Val=nullptr.  Isn't that just &get?  If yes than I think that's what we should use for it since we're not setting up the entries.

How much of this you want to put in this patch or leave it for later improvements (by you or others) I leave it to you.  My goal is to use the interfaces that return mutable references as little as possible and restrain the current ones to return const & as much as possible.


https://reviews.llvm.org/D23169