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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1762
+  }
+
+  VPBasicBlock *Preheader = Plan.getEntry()->getEntryBasicBlock();
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122096