[PATCH] D122096: [VPlan] Expand induction step in VPlan pre-header.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8171
 static VPWidenIntOrFpInductionRecipe *
 createWidenInductionRecipe(PHINode *Phi, Instruction *PhiOrTrunc,
                            VPValue *Start, const InductionDescriptor &IndDesc,
----------------
Ayal wrote:
> Strictly speaking, this may create more than the WidenInductionRecipe it returns. Perhaps rename, e.g., using plural (Recipes)? Worth documenting.
Updated and added a comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1300
+  O << ", ";
+  getOperand(1)->printAsOperand(O, SlotTracker);
 }
----------------
Ayal wrote:
> getStepValue()?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1762
+  }
+
+  VPBasicBlock *Preheader = Plan.getEntry()->getEntryBasicBlock();
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Add comment; searching if a recipe has already been created for Expr, and return it if so, to save redundant duplication(?)
> > > Can alternatively cache Expr's that have recipes.
> > > 
> > > Should we also check if Constant or Unknown Expr's were assigned VPValues, and return them, instead of creating new ones - potentially leading to a leak?
> > Updated to cache scev expressions.
> Ah, creating multiple VPValues for same underlying External Def Value should not lead to a leak; VPlan's VPExternalDefs should hold them all, i.e., addExternalDef()'s insert should succeed. But perhaps better to ensure each External Def Value is wrapped by a VPValue only once (see TODO next to addExternalDef()), by having Plan.getOrCreateExternalDef(Value*), potentially indexing VPExternalDefs by their underlying Value?
> 
> Perhaps better to simplify initial recipe creation here, and deduplicate (SCEV) recipes by a VPlan2VPlan optimization later (scanning the preheader with a SCEV2Recipe map)?
I put up D123700 to address the TODO.

> Perhaps better to simplify initial recipe creation here, and deduplicate (SCEV) recipes by a VPlan2VPlan optimization later (scanning the preheader with a SCEV2Recipe map)?

Update the patch, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122096