[PATCH] D34473: [LV] Changing the interface of ValueMap

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

Hi Ayal,

Sorry for the long turn-around on this. This looks mostly fine to me. I think it would make sense to commit this prior to the VPlan patch since it's conceptually a separate change. Committing first would also ensure that we keep the interface clean, throughout.

Michael, do you have any issues with moving to this new interface for the ValueMap?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:685-687
-    friend const VectorParts &InnerLoopVectorizer::getVectorValue(Value *V);
-    friend Value *InnerLoopVectorizer::getScalarValue(Value *V, unsigned Part,
-                                                      unsigned Lane);
----------------
Don't we still need to declare this functions, and the others, as friends?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:699
+        for (unsigned Part = 0; Part < UF; ++Part)
+          Entry[Part].resize(VF);
+        ScalarMapStorage[Key] = Entry;
----------------
`Entry[Part].resize(VF)` null initializes the values, right?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2449-2450
+    VectorLoopValueMap.setVectorValue(EntryVal, Part, LastInduction);
+    if (isa<TruncInst>(EntryVal))
+      addMetadata(LastInduction, EntryVal);
     LastInduction = cast<Instruction>(addFastMathFlag(
----------------
It might be more efficient in the common case if we pulled this out of the loop. Though, I don't really have a strong preference.

```
if (isa<TruncInst>(EntryVal))
  for (unsigned Part = 0; Part < UF; ++Part)
    addMetadata(getOrCreateVectorValue(EntryVal, Part);
```

Hopefully, I've understood the interface correctly...


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2563-2564
+      VectorLoopValueMap.setVectorValue(EntryVal, Part, EntryPart);
+      if (Trunc)
+        addMetadata(EntryPart, Trunc);
+    }
----------------
Same as above.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2706
   // vector form, we will construct the vector values on demand.
-  if (VectorLoopValueMap.hasScalar(V)) {
+  if (VectorLoopValueMap.hasAnyScalarValue(V)) {
 
----------------
Do we need to check/assert that the map contains a non-null entry for each Lane? Is it possible that some of the entries would be null?

Maybe uniform-after-vectorization values have null entries for `VF > 1`? If this is still the case, it might be useful to re-think that choice. It might be simpler to just broadcast the `VF == 1` value to the other lanes instead of leaving them null.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4038
+  Builder.SetInsertPoint(
+      cast<Instruction>(VectorLoopValueMap.getVectorValue(Phi, 0)));
 
----------------
Why do we use `VectorLoopValueMap.getVectorValue()` here instead of `getOrCreateVectorValue()`? Is it because we know the Phi must exist in the map already? This makes sense to me.

I think in the original code, we had to use `VectorLoopValueMap.getVector()` to get a non-const reference.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4219
 
-  VectorParts &RdxParts = VectorLoopValueMap.getVector(LoopExitInst);
+  VectorParts RdxParts(UF);
   setDebugLocFromInst(Builder, LoopExitInst);
----------------
Can't we just use `VectorLoopValueMap.getVectorValue(LoopExitInst, Part)`, etc.?


https://reviews.llvm.org/D34473