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

[Florian Hahn via Phabricator via llvm-commits]

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

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



================
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());
----------------
Ayal wrote:
> 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).
Thanks, should be fixed with bcc9c5d959b9.


================
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()) ||
----------------
Ayal wrote:
> nit: comment deserves updating? (irrespective of this patch)
Done in 239b52d4b6f6, thanks!


================
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:
> 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)
Oh right, this was independent of the patch! Updated in aa16689f82a0, thanks!


================
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(); }
 };
----------------
Ayal wrote:
> nit: should we introduce a `hasDefiningRecipe()` returning a boolean to better support cases that currently check `if (VPV->getDefiningRecipe())`?
Done in 55f56cdc3329, thanks!


================
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()));
----------------
Ayal wrote:
> nit: check instead that it's not null? (irrespective of this patch)
Thanks, should all be addressed in b52d328ee8aa.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136068