[PATCH] D147892: [VPlan] Unify Value2VPValue and VPExternalDefs maps (NFCI).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 9 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8121
+  VPValue *EdgeMask = Plan.getOrAddExternalVPValue(BI->getCondition());
   assert(EdgeMask && "No Edge Mask found for condition");
 
----------------
Ayal wrote:
> nit (Independent of this patch): assert seems redundant - getOrAdd*() cannot return null?
Yeah, I can remove it separately.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8849
         ExitPhi.getIncomingValueForBlock(ExitingBB);
-    VPValue *V = Plan.getOrAddVPValue(IncomingValue, true);
+    VPValue *V = Plan.getOrAddExternalVPValue(IncomingValue, true);
     Plan.addLiveOut(&ExitPhi, V);
----------------
Ayal wrote:
> nit (Independent of this patch): should this case add a
> 
> ```
> // FIXME: Should not rely on getVPValue at this point.
> 
> ```
> referring to D108573?
Update the code, the new `getVPValueOrAddLiveIn` only doesn't require the parameter, as live-ins can be added regardless.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10450
+          VPValue *StartVal = BestEpiPlan.getOrAddExternalVPValue(
+              ResumeV, /*OverrideAllowed=*/true);
           cast<VPHeaderPHIRecipe>(&R)->setStartValue(StartVal);
----------------
Ayal wrote:
> Explain why override is allowed? Add above FIXME?
> 
> 
Update getOrAddLiveIn to be allowed generally.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2246
+  /// Contains all the external definitions created for this VPlan. External
+  /// definitions are VPValues that hold a pointer to their underlying IR.
+  SmallVector<VPValue *, 16> VPExternalDefs;
----------------
Ayal wrote:
> Still only to remember whom to destroy? Worth retaining ToFree suffix?
Retained, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2250
   /// Indicates whether it is safe use the Value2VPValue mapping or if the
   /// mapping cannot be used any longer, because it is stale.
   bool Value2VPValueEnabled = true;
----------------
Ayal wrote:
> For live-ins/external-defs, the mapping is always intact?
> 
> Perhaps distinguish between getting a VPValue which may be a recipe, for which the mapping will become stale, and getting-or-adding a live-in VPValue?
Agreed, updated to always allow querying and adding live-ins.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2327
 
-  /// Get the existing or add a new external definition for \p V.
-  VPValue *getOrAddExternalDef(Value *V) {
-    auto I = VPExternalDefs.insert({V, nullptr});
-    if (I.second)
-      I.first->second = new VPValue(V);
-    return I.first->second;
-  }
-
-  void addVPValue(Value *V) {
-    assert(Value2VPValueEnabled &&
-           "IR value to VPValue mapping may be out of date!");
-    assert(V && "Trying to add a null Value to VPlan");
-    assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
-    VPValue *VPV = new VPValue(V);
-    Value2VPValue[V] = VPV;
-    VPValuesToFree.push_back(VPV);
-  }
-
   void addVPValue(Value *V, VPValue *VPV) {
     assert(Value2VPValueEnabled && "Value2VPValue mapping may be out of date!");
----------------
Ayal wrote:
> Note that addVPValue(Value*, VPValue*) remains intact, it is only addVPValue(Value*) which is renamed addExternalVPValue(Value*) below, as the former adds recipe VPValues whereas the latter adds live-in ones.
`addExternalVPValue` has been removed and `addVPValue(Value*, VPValue*) ` has been updated to also handle the live-in case in the assert


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2336
   /// checking whether it is safe to query VPValues using IR Values.
   VPValue *getVPValue(Value *V, bool OverrideAllowed = false) {
     assert((OverrideAllowed || isa<Constant>(V) || Value2VPValueEnabled) &&
----------------
Ayal wrote:
> Note that getVPValue() remains intact, used to retrieve both live-ins and recipe VPValues, although if used to retrieve live-ins it should disregard Value2VPValueEnabled - the obvious case being constants checked below.
Updated to allow retrieving live-ins in the assert.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2345
+  /// Adds a VPValue wrapping \p V as external definition.
+  void addExternalVPValue(Value *V) {
+    assert(V && "Trying to add a null Value to VPlan");
----------------
Ayal wrote:
> This adds a live-in, so indeed should be enabled always, regardless of Value2VPValueEnabled; although currently addVPValue(Value*) which adds a live-in does check Value2VPValueEnabled.
This is gone now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2356
+  /// whether it is safe to query VPValues using IR Values.
+  VPValue *getOrAddExternalVPValue(Value *V, bool OverrideAllowed = false) {
     assert(V && "Trying to get or add the VPValue of a null Value");
----------------
Ayal wrote:
> getOrAddExternalVPValue() deals with live-in VPValues only, why should it consider Value2VPValueEnabled?
Removed assert and renamed `getVPValueOrAddLiveIn` and  updated the code to use  existing `addVPValue` and `getVPValue`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147892/new/

https://reviews.llvm.org/D147892