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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

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.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:111
 #endif
 
+VPRecipeBase *VPValue::getDef() { return cast_or_null<VPRecipeBase>(Def); }
----------------
Documentation?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:112
 
+VPRecipeBase *VPValue::getDef() { return cast_or_null<VPRecipeBase>(Def); }
+const VPRecipeBase *VPValue::getDef() const {
----------------
Empty line separator?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1177
   /// Returns the backedge value as a recipe. The backedge value is guaranteed
   /// to be a recipe.
+  VPRecipeBase *getBackedgeRecipe() { return getBackedgeValue()->getDef(); }
----------------
Rephrase comment and/or return a reference?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136068