[PATCH] D32871: [LV] Using VPlan to model the vectorized code and drive its transformation

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:710-748
+    /// Returns (and creates if needed) ScalarParts that are mapped to the given
+    /// \p Key.
+    ScalarParts &getOrCreateScalar(Value *Key, unsigned Lanes, unsigned Parts) {
+      assert(Lanes == VF && Parts == UF && "Wrong dimensions for ScalarParts.");
+      if (!hasScalar(Key)) {
+        ScalarParts Entry(Parts);
+        for (unsigned Part = 0; Part < Parts; ++Part)
----------------
The interface to the vector and scalar instruction maps seems to be getting more complex and confusing with this patch. Do we actually need all of these additional functions? How do they interact with the existing ones (init* and get*)? Are the existing ones still necessary?

I think eventually we want to move this interface out of ILV and into a standalone storage class.

But it looks like one thing that changes with this patch is that the maps can be "incomplete" at a given instance (i.e., you call initVector with an Entry that has been initialized with null pointers, then later I assume use the new setters to complete the mapping). Currently, I think we only add a mapping to the storage once all the entry values are completely defined. Is there a reason this needs to change (it's fine if it does, I'm just trying to understand)? An advantage of the current approach would be that if a key exists in the map, we know that all of the entry values should be non-null (I think we've even caught some bugs this way).

getVectorValue was made to return a constant reference to enforce the idea that the entry values shouldn't be changed once added to the map (although getVector still had to exist for handling reductions, so this never really fit well, admittedly).

Sorry for the rambling comment - I think we need to better document the interface and remove the functions that are no longer necessary.


https://reviews.llvm.org/D32871