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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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)
----------------
mssimpso wrote:
> 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.
Yes, the existing interfaces are still needed. This patch introduces the ability for scalarizeInstruction() to generate a single replica when needed for scalarized and predicated instructions, *alongside* the existing ability of generating all replica's for other scalarized instructions.
 
Yes, the newly added getOrCreateScalar() supports setting one replica at a time, and as such renders this mapping "incomplete" until the last replica is generated. In the case of generating scalarized and predicated instructions with VPlan, each scalar replica is generated separately as part of replicating the entire region, under the (existing) assumption that all uses of one replica are generated before generating the next replica.

An alternative interface which may be simpler and clearer is to only support setting and getting a single Value per part, or per part-and-lane. Setting a value can assert that no value has already been set; this assert can be overridden when needed using a specialized "resetting" method. This keeps the implementation of *MapStorage internal to ValueMap, albeit potentially being less efficient as it avoids batching (if not inlined). If so, it may be better done in a separate patch. Sounds reasonable?

Current interface:
```
ILV:
  const VectorParts &getVectorValue(Value *V)
  Value *getScalarValue(Value *V, unsigned Part, unsigned Lane)
ValueMap:
  bool hasVector(Value *Key)
  bool hasScalar(Value *Key)
  const VectorParts &initVector(Value *Key, const VectorParts &Entry)
  const ScalarParts &initScalar(Value *Key, const ScalarParts &Entry)
  VectorParts &getVector(Value *Key)
```

Alternative interface:
```
ILV:
  Value *getScalarValue(Value *V, const VPIteration &I) // Get scalarized or extract vectorized.
  Value *getVectorValue(Value *V, unsigned Part) // Get vectorized or pack/broadcast scalarized/uniform.

ValueMap:
  bool hasScalarValue(Value *V, const VPIteration &I)
  bool hasVectorValue(Value *V, unsigned Part)
  void setScalarValue(Value *V, const VPIteration &I, Value *Scalar) // Asserts value not already set.
  void setVectorValue(Value *V, unsigned Part, Value *Vector) // Asserts value not already set.
  void resetScalarValue(Value *V, const VPIteration &I, Value *Scalar) // Can assert value already set.
  void resetVectorValue(Value *V, unsigned Part, Value *Vector) // Can assert value already set.
```

Agreed, this mapping should eventually move out of ILV and into a standalone storage class, similar to the VPBB2IRBB mapping in TransformState.CFGState. We're trying to breakdown these and other changes into separate patches.


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

Seconded ;-)


https://reviews.llvm.org/D32871