[VPlan] Update VPValue::getDef to return VPRecipeBase* (NFC).

[fhahn]

Summary

The return value of getDef is guaranteed to be a VPRecipeBase and all
users can also accept a VPRecipeBase *. Most users actually case to
VPRecipeBase or a specific recipe before using it, so this change
removes a number of redundant casts.

[Ayal]

A more accurate name for getDef() may then be getDefiningRecipe(), as in MLIR's Value::getDefiningOp().

Using cast_or_null() effectively implies that VPDef is a pure base-class having VPRecipeBase as its sole direct subclass. If this is (and will continue to be) the case, should VPDef's constructor be non-public, and VPDef be an internal subclass of VPRecipeBase rather than a base-class thereof? The original intention was for VPDef to be analogous to VPUser, but live-in's are represented with VPValue's having no VPDef whereas live-out's are represented with the VPLiveOut subclass of VPUser - sibling of VPRecipeBase.

llvm/lib/Transforms/Vectorize/VPlan.cpp

111

Documentation?

112

Empty line separator?

1111

Append the newly created recipe directly instead of upcasting it to VPValue and back?

llvm/lib/Transforms/Vectorize/VPlan.h

1177

Rephrase comment and/or return a reference?

[fhahn]

In D136068#3877832, @Ayal wrote:

A more accurate name for getDef() may then be getDefiningRecipe(), as in MLIR's Value::getDefiningOp().

Updated, thanks!

Using cast_or_null() effectively implies that VPDef is a pure base-class having VPRecipeBase as its sole direct subclass. If this is (and will continue to be) the case, should VPDef's constructor be non-public, and VPDef be an internal subclass of VPRecipeBase rather than a base-class thereof? The original intention was for VPDef to be analogous to VPUser, but live-in's are represented with VPValue's having no VPDef whereas live-out's are represented with the VPLiveOut subclass of VPUser - sibling of VPRecipeBase.

Sounds good as a separate refactoring!

[fhahn]

llvm/lib/Transforms/Vectorize/VPlan.cpp

111

added in the header, thanks

112

done, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h

1177

updated to return a reference

[fhahn]

ping :)

[Ayal]

This looks good to me, thanks. Adding various nits, mostly irrespective of this patch.

In D136068#3912170, @fhahn wrote:

In D136068#3877832, @Ayal wrote:

A more accurate name for getDef() may then be getDefiningRecipe(), as in MLIR's Value::getDefiningOp().

Updated, thanks!

Very good, thanks!

Using cast_or_null() effectively implies that VPDef is a pure base-class having VPRecipeBase as its sole direct subclass. If this is (and will continue to be) the case, should VPDef's constructor be non-public, and VPDef be an internal subclass of VPRecipeBase rather than a base-class thereof? The original intention was for VPDef to be analogous to VPUser, but live-in's are represented with VPValue's having no VPDef whereas live-out's are represented with the VPLiveOut subclass of VPUser - sibling of VPRecipeBase.

Sounds good as a separate refactoring!

Sure, a todo can be left behind.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1053–1054

Comment: it's fine to retain the Def suffix of variable names even though they are no longer VPDef but are now VPRecipeBase, because they represent a Defining recipe.

8489

nit: the return of cast_or_null may be null, but code below dereferences RepR expecting it to be non-null (irrespective of this patch).

9900–9901

nit: comment deserves updating? (irrespective of this patch)

llvm/lib/Transforms/Vectorize/VPlan.cpp

1111

Append the newly created recipe directly instead of upcasting it to VPValue and back?

i.e.,

VPExpandSCEVRecipe *StepR = new VPExpandSCEVRecipe(Expr, SE);
Preheader->appendRecipe(StepR);
return StepR;

?
(somewhat irrespective of this patch)

llvm/lib/Transforms/Vectorize/VPlanValue.h

205

nit: should we introduce a hasDefiningRecipe() returning a boolean to better support cases that currently check if (VPV->getDefiningRecipe())?

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

816 ↗

(On Diff #473655)

nit: check instead that it's not null? (irrespective of this patch)

817 ↗

(On Diff #473655)

nit: dyn_cast is redundant.

845 ↗

(On Diff #473655)

ditto x 2, here and below

[fhahn]

Thanks for taking a look! The independent suggestions should have been addressed separately.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

8489

Thanks, should be fixed with bcc9c5d959b9.

9900–9901

Done in 239b52d4b6f6, thanks!

llvm/lib/Transforms/Vectorize/VPlan.cpp

1111

Oh right, this was independent of the patch! Updated in aa16689f82a0, thanks!

llvm/lib/Transforms/Vectorize/VPlanValue.h

205

Done in 55f56cdc3329, thanks!

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

816 ↗

(On Diff #473655)

Thanks, should all be addressed in b52d328ee8aa.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

while (!Worklist.empty()) {
       // load/store. Collect recipe if its underlying instruction has
       // poison-generating flags.
       Instruction *Instr = CurRec->getUnderlyingInstr();
       if (Instr && Instr->hasPoisonGeneratingFlags())
         State.MayGeneratePoisonRecipes.insert(CurRec);
 
       // Add new definitions to the worklist.
       for (VPValue *operand : CurRec->operands())
-        if (VPDef *OpDef = operand->getDef())
-          Worklist.push_back(cast<VPRecipeBase>(OpDef));
+        if (VPRecipeBase *OpDef = operand->getDef())
+          Worklist.push_back(OpDef);
     }
   });
 
   // Traverse all the recipes in the VPlan and collect the poison-generating
   // recipes in the backward slice starting at the address of a VPWidenRecipe or
   // VPInterleaveRecipe.
   auto Iter = depth_first(
       VPBlockRecursiveTraversalWrapper<VPBlockBase *>(State.Plan->getEntry()));
   for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(Iter)) {
     for (VPRecipeBase &Recipe : *VPBB) {
       if (auto *WidenRec = dyn_cast<VPWidenMemoryInstructionRecipe>(&Recipe)) {
         Instruction &UnderlyingInstr = WidenRec->getIngredient();
-        VPDef *AddrDef = WidenRec->getAddr()->getDef();
+        VPRecipeBase *AddrDef = WidenRec->getAddr()->getDef();
         if (AddrDef && WidenRec->isConsecutive() &&
             Legal->blockNeedsPredication(UnderlyingInstr.getParent()))
-          collectPoisonGeneratingInstrsInBackwardSlice(
-              cast<VPRecipeBase>(AddrDef));
+          collectPoisonGeneratingInstrsInBackwardSlice(AddrDef);
       } else if (auto *InterleaveRec = dyn_cast<VPInterleaveRecipe>(&Recipe)) {
-        VPDef *AddrDef = InterleaveRec->getAddr()->getDef();
+        VPRecipeBase *AddrDef = InterleaveRec->getAddr()->getDef();
         if (AddrDef) {
           // Check if any member of the interleave group needs predication.
           const InterleaveGroup<Instruction> *InterGroup =
               InterleaveRec->getInterleaveGroup();
           bool NeedPredication = false;
           for (int I = 0, NumMembers = InterGroup->getNumMembers();
                I < NumMembers; ++I) {
             Instruction *Member = InterGroup->getMember(I);
             if (Member)
               NeedPredication |=
                   Legal->blockNeedsPredication(Member->getParent());
           }
 
           if (NeedPredication)
-            collectPoisonGeneratingInstrsInBackwardSlice(
-                cast<VPRecipeBase>(AddrDef));
+            collectPoisonGeneratingInstrsInBackwardSlice(AddrDef);
         }
       }
     }
   }
 }
 
 PHINode *InnerLoopVectorizer::getReductionResumeValue(
     const RecurrenceDescriptor &RdxDesc) {
VPBasicBlock *VPRecipeBuilder::handleReplication(
   // Find if I uses a predicated instruction. If so, it will use its scalar
   // value. Avoid hoisting the insert-element which packs the scalar value into
   // a vector value, as that happens iff all users use the vector value.
   for (VPValue *Op : Recipe->operands()) {
     auto *PredR = dyn_cast_or_null<VPPredInstPHIRecipe>(Op->getDef());
     if (!PredR)
       continue;
     auto *RepR =
         cast_or_null<VPReplicateRecipe>(PredR->getOperand(0)->getDef());
     assert(RepR->isPredicated() &&
            "expected Replicate recipe to be predicated");
     RepR->setAlsoPack(false);
   }
 
   // Finalize the recipe for Instr, first if it is not predicated.
   if (!IsPredicated) {
     LLVM_DEBUG(dbgs() << "LV: Scalarizing:" << *I << "\n");
for (Instruction &I : BB->instructionsWithoutDebug()) {
       if (auto RecipeOrValue = RecipeBuilder.tryToCreateWidenRecipe(
               Instr, Operands, Range, VPBB, Plan)) {
         // If Instr can be simplified to an existing VPValue, use it.
         if (RecipeOrValue.is<VPValue *>()) {
           auto *VPV = RecipeOrValue.get<VPValue *>();
           Plan->addVPValue(Instr, VPV);
           // If the re-used value is a recipe, register the recipe for the
           // instruction, in case the recipe for Instr needs to be recorded.
-          if (auto *R = dyn_cast_or_null<VPRecipeBase>(VPV->getDef()))
+          if (VPRecipeBase *R = VPV->getDef())
             RecipeBuilder.setRecipe(Instr, R);
           continue;
         }
         // Otherwise, add the new recipe.
         VPRecipeBase *Recipe = RecipeOrValue.get<VPRecipeBase *>();
         for (auto *Def : Recipe->definedValues()) {
           auto *UV = Def->getUnderlyingValue();
           Plan->addVPValue(UV, Def);
if (CM.foldTailByMasking()) {
     for (VPRecipeBase &R :
          Plan->getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
       VPReductionPHIRecipe *PhiR = dyn_cast<VPReductionPHIRecipe>(&R);
       if (!PhiR || PhiR->isInLoop())
         continue;
       VPValue *Cond =
           RecipeBuilder.createBlockInMask(OrigLoop->getHeader(), Plan);
       VPValue *Red = PhiR->getBackedgeValue();
-      assert(cast<VPRecipeBase>(Red->getDef())->getParent() != LatchVPBB &&
+      assert(Red->getDef()->getParent() != LatchVPBB &&
              "reduction recipe must be defined before latch");
       Builder.createNaryOp(Instruction::Select, {Cond, Red, PhiR});
     }
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 void VPInterleaveRecipe::print(raw_ostream &O, const Twine &Indent,
if (VF.isScalar()) {
     return ScalarValue;
   }
 
   bool IsUniform = vputils::isUniformAfterVectorization(Def);
 
   unsigned LastLane = IsUniform ? 0 : VF.getKnownMinValue() - 1;
   // Check if there is a scalar value for the selected lane.
   if (!hasScalarValue(Def, {Part, LastLane})) {
     // At the moment, VPWidenIntOrFpInductionRecipes can also be uniform.
     assert((isa<VPWidenIntOrFpInductionRecipe>(Def->getDef()) ||
             isa<VPScalarIVStepsRecipe>(Def->getDef())) &&
            "unexpected recipe found to be invariant");
     IsUniform = true;
     LastLane = 0;
   }
 
   auto *LastInst = cast<Instruction>(get(Def, {Part, LastLane}));
   // Set the insert point after the last scalarized instruction or after the

llvm/lib/Transforms/Vectorize/VPlan.h

#endif
   }
 
   /// Returns the incoming value from the loop backedge.
   VPValue *getBackedgeValue() {
     return getOperand(1);
   }
 
   /// Returns the backedge value as a recipe. The backedge value is guaranteed
   /// to be a recipe.
-  VPRecipeBase *getBackedgeRecipe() {
-    return cast<VPRecipeBase>(getBackedgeValue()->getDef());
-  }
+  VPRecipeBase *getBackedgeRecipe() { return getBackedgeValue()->getDef(); }
 };
 
 class VPWidenPointerInductionRecipe : public VPHeaderPHIRecipe {
   const InductionDescriptor &IndDesc;
 
   bool IsScalarAfterVectorization;
 
 public:
VPValue *getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr,
                                        ScalarEvolution &SE);
 
 /// Returns true if \p VPV is uniform after vectorization.
 inline bool isUniformAfterVectorization(VPValue *VPV) {
   // A value defined outside the vector region must be uniform after
   // vectorization inside a vector region.
   if (VPV->isDefinedOutsideVectorRegions())
     return true;
-  VPDef *Def = VPV->getDef();
+  VPRecipeBase *Def = VPV->getDef();
   assert(Def && "Must have definition for value defined inside vector region");
   if (auto Rep = dyn_cast<VPReplicateRecipe>(Def))
     return Rep->isUniform();
   return false;
 }
 } // end namespace vputils
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_VECTORIZE_VPLAN_H

llvm/lib/Transforms/Vectorize/VPlan.cpp

 void VPDef::dump() const {
   const VPRecipeBase *Instr = dyn_cast_or_null<VPRecipeBase>(this);
   VPSlotTracker SlotTracker(
       (Instr && Instr->getParent()) ? Instr->getParent()->getPlan() : nullptr);
   print(dbgs(), "", SlotTracker);
   dbgs() << "\n";
 }
 #endif
 
+VPRecipeBase *VPValue::getDef() { return cast_or_null<VPRecipeBase>(Def); }
+const VPRecipeBase *VPValue::getDef() const {
+  return cast_or_null<VPRecipeBase>(Def);
+}
+
 // Get the top-most entry block of \p Start. This is the entry block of the
 // containing VPlan. This function is templated to support both const and non-const blocks
 template <typename T> static T *getPlanEntry(T *Start) {
   T *Next = Start;
   T *Current = Start;
   while ((Next = Next->getParent()))
     Current = Next;
 

 VPValue *vputils::getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr,
                                                 ScalarEvolution &SE) {
   if (auto *E = dyn_cast<SCEVConstant>(Expr))
     return Plan.getOrAddExternalDef(E->getValue());
   if (auto *E = dyn_cast<SCEVUnknown>(Expr))
     return Plan.getOrAddExternalDef(E->getValue());
 
   VPBasicBlock *Preheader = Plan.getEntry()->getEntryBasicBlock();
   VPValue *Step = new VPExpandSCEVRecipe(Expr, SE);
-  Preheader->appendRecipe(cast<VPRecipeBase>(Step->getDef()));
+  Preheader->appendRecipe(Step->getDef());
   return Step;
 }

llvm/lib/Transforms/Vectorize/VPlanValue.h

bool hasMoreThanOneUniqueUser() {
     auto Current = std::next(user_begin());
     while (Current != user_end() && *user_begin() == *Current)
       Current++;
     return Current != user_end();
   }
 
   void replaceAllUsesWith(VPValue *New);
 
-  VPDef *getDef() { return Def; }
-  const VPDef *getDef() const { return Def; }
+  VPRecipeBase *getDef();
+  const VPRecipeBase *getDef() const;
 
   /// Returns the underlying IR value, if this VPValue is defined outside the
   /// scope of VPlan. Returns nullptr if the VPValue is defined by a VPDef
   /// inside a VPlan.
   Value *getLiveInIRValue() {
     assert(!getDef() &&
            "VPValue is not a live-in; it is defined by a VPDef inside a VPlan");
     return getUnderlyingValue();
   }
   const Value *getLiveInIRValue() const {
     assert(!getDef() &&
            "VPValue is not a live-in; it is defined by a VPDef inside a VPlan");
     return getUnderlyingValue();
   }
 
   /// Returns true if the VPValue is defined outside any vector regions, i.e. it
   /// is a live-in value.
   /// TODO: Also handle recipes defined in pre-header blocks.
   bool isDefinedOutsideVectorRegions() const { return !getDef(); }
 };
 
 typedef DenseMap<Value *, VPValue *> Value2VPValueTy;
 typedef DenseMap<VPValue *, Value *> VPValue2ValueTy;
 
 raw_ostream &operator<<(raw_ostream &OS, const VPValue &V);
 
 /// This class augments VPValue with operands which provide the inverse def-use
class VPDef {
   /// Subclass identifier (for isa/dyn_cast).
   const unsigned char SubclassID;
 
   /// The VPValues defined by this VPDef.
   TinyPtrVector<VPValue *> DefinedValues;
 
   /// Add \p V as a defined value by this VPDef.
   void addDefinedValue(VPValue *V) {
-    assert(V->getDef() == this &&
+    assert(V->Def == this &&
            "can only add VPValue already linked with this VPDef");
     DefinedValues.push_back(V);
   }
 
   /// Remove \p V from the values defined by this VPDef. \p V must be a defined
   /// value of this VPDef.
   void removeDefinedValue(VPValue *V) {
-    assert(V->getDef() == this &&
-           "can only remove VPValue linked with this VPDef");
+    assert(V->Def == this && "can only remove VPValue linked with this VPDef");
     assert(is_contained(DefinedValues, V) &&
            "VPValue to remove must be in DefinedValues");
     erase_value(DefinedValues, V);
     V->Def = nullptr;
   }
 
 public:
   /// An enumeration for keeping track of the concrete subclass of VPRecipeBase