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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This indeed deserves cleaning up!
Would it be correct and reasonable to rename "External[Def]" >> "LiveIn[VPValue]" throughout, as in !hasDefiningRecipe()?
Value2VPValueEnabled presumably applies to recipe'd VPValues only, i.e., excluding live-in VPValues? Better decrease allowed overriding cases, rather than increase them.



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


================
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);
----------------
nit (Independent of this patch): should this case add a

```
// FIXME: Should not rely on getVPValue at this point.

```
referring to D108573?


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




================
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;
----------------
Still only to remember whom to destroy? Worth retaining ToFree suffix?


================
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;
----------------
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?


================
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!");
----------------
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.


================
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) &&
----------------
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.


================
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");
----------------
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.


================
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");
----------------
getOrAddExternalVPValue() deals with live-in VPValues only, why should it consider Value2VPValueEnabled?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2362
+    assert((OverrideAllowed || Value2VPValueEnabled ||
+            !VPV->getDefiningRecipe()) &&
+           "IR value to VPValue mapping may be out of date!");
----------------
Indeed, VPV must not have a defining recipe when expecting to get a live-in/external VPValue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147892