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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

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

In D136068#3912170 <https://reviews.llvm.org/D136068#3912170>, @fhahn wrote:

> In D136068#3877832 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1053
       for (VPValue *operand : CurRec->operands())
-        if (VPDef *OpDef = operand->getDef())
-          Worklist.push_back(cast<VPRecipeBase>(OpDef));
+        if (VPRecipeBase *OpDef = operand->getDefiningRecipe())
+          Worklist.push_back(OpDef);
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8492
       continue;
-    auto *RepR =
-        cast_or_null<VPReplicateRecipe>(PredR->getOperand(0)->getDef());
+    auto *RepR = cast_or_null<VPReplicateRecipe>(
+        PredR->getOperand(0)->getDefiningRecipe());
----------------
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).


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9882
   if (!hasScalarValue(Def, {Part, LastLane})) {
     // At the moment, VPWidenIntOrFpInductionRecipes can also be uniform.
+    assert((isa<VPWidenIntOrFpInductionRecipe>(Def->getDefiningRecipe()) ||
----------------
nit: comment deserves updating? (irrespective of this patch)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1111
   VPBasicBlock *Preheader = Plan.getEntry()->getEntryBasicBlock();
   VPValue *Step = new VPExpandSCEVRecipe(Expr, SE);
+  Preheader->appendRecipe(Step->getDef());
----------------
Ayal wrote:
> Append the newly created recipe directly instead of upcasting it to VPValue and back?
> 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)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:207
   /// TODO: Also handle recipes defined in pre-header blocks.
-  bool isDefinedOutsideVectorRegions() const { return !getDef(); }
+  bool isDefinedOutsideVectorRegions() const { return !getDefiningRecipe(); }
 };
----------------
nit: should we introduce a `hasDefiningRecipe()` returning a boolean to better support cases that currently check `if (VPV->getDefiningRecipe())`?


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:816
   VPValue *VPV = &Recipe;
-  EXPECT_TRUE(isa<VPRecipeBase>(VPV->getDef()));
-  EXPECT_EQ(&Recipe, dyn_cast<VPRecipeBase>(VPV->getDef()));
+  EXPECT_TRUE(isa<VPRecipeBase>(VPV->getDefiningRecipe()));
+  EXPECT_EQ(&Recipe, dyn_cast<VPRecipeBase>(VPV->getDefiningRecipe()));
----------------
nit: check instead that it's not null? (irrespective of this patch)


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:817
+  EXPECT_TRUE(isa<VPRecipeBase>(VPV->getDefiningRecipe()));
+  EXPECT_EQ(&Recipe, dyn_cast<VPRecipeBase>(VPV->getDefiningRecipe()));
 
----------------
nit: dyn_cast is redundant.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:845
+  EXPECT_TRUE(isa<VPRecipeBase>(VPV->getDefiningRecipe()));
+  EXPECT_EQ(&WidenSelectR, dyn_cast<VPRecipeBase>(VPV->getDefiningRecipe()));
 
----------------
ditto x 2, here and below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136068