[PATCH] D147964: [VPlan] Introduce new entry block to VPlan for early SCEV expansion.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:571
+  /// Used to set the trip count after ILV's construction and after the
+  /// preheader block has been executed.
+  void setTripCount(Value *TC) { TripCount = TC; }
----------------
nit: also worth clarifying that the same trip count - that of the original loop - is (generated once and) set for both ILV's of main and epilog loops?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3260
+       [ ] <-- old preheader - loop iteration number check and SCEVs in Plans
+    /   |       preheader are expanded here.
    /    v
----------------
nit: "SCEVs in Plans [preheader] are expanded here" >> "all SCEV expansions needed by vectorized loops"?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3360
       VPValue *StepVPV = Plan.getSCEVExpansion(II.getStep());
-      Value *Step = StepVPV->getDefiningRecipe() ? State.get(StepVPV, 0)
+      Value *Step = StepVPV->getDefiningRecipe() ? State.get(StepVPV, {0, 0})
                                                  : StepVPV->getLiveInIRValue();
----------------
Curious about this needed change in handling invariant "live-ins"?
> As a consequence some places needed to be updated to specifically requests Part & Lane 0 to handle live-ins properly in VPTransformState::get.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7692
+    State.Builder.SetInsertPoint(OrigLoop->getLoopPreheader()->getTerminator());
+    BestVPlan.getPreheader()->execute(&State);
+  }
----------------
How/do we prevent expanding trip count SCEV again needlessly when vectorizing epilog loop?

Other SCEV expands in preheader are connected directly to distinct VPUsers so need be expanded even if common to both main and epilog loop, hopefully cse'd later?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7695
+  if (!ILV.getTripCount())
+    ILV.setTripCount(State.get(BestVPlan.getTripCount(), {0, 0}));
 
----------------
nit: worth a comment that the 'else' case refers to vectorizing the epilog loop, whose ILV borrows the trip count from that of the main loop's ILV (the latter being set here)?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8847
+      vputils::getOrCreateVPValueForSCEVExpr(*Plan, TripCount, *PSE.getSE());
+  Plan->setTripCount(TripCountVPV);
+  return Plan;
----------------
nit: there is a bit of chicken-and-egg when defining TripCount and Plan. Can let Plan's constructor create its own TripCount - it should be immutable/unsettable.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9825
   if (!hasScalarValue(Def, {Part, LastLane})) {
     // At the moment, VPWidenIntOrFpInductionRecipes and VPScalarIVStepsRecipes can also be uniform.
     assert((isa<VPWidenIntOrFpInductionRecipe>(Def->getDefiningRecipe()) ||
----------------
nits: "... and VPExpandSCEVRecipes". "can" refers only to WidenIntOrFpInductions?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10415
 
+        EpilogILV.setTripCount(MainILV.getTripCount());
+
----------------
nit: worth commenting about setting trip count explicitly here/now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1109
+    for (VPValue *Def : Recipe.definedValues())
+      assignSlot(Def);
+
----------------
nit: have assignSlots(VPBB)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1131-1133
   if (auto *Expanded = Plan.getSCEVExpansion(Expr))
     return Expanded;
   VPValue *Expanded = nullptr;
----------------
nit: where were the caching get/addSCEVExpansion() added? (Independent of this patch).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1139-1140
   else {
-    VPBasicBlock *Preheader = Plan.getEntry();
     Expanded = new VPExpandSCEVRecipe(Expr, SE);
-    Preheader->appendRecipe(Expanded->getDefiningRecipe());
+    Plan.getPreheader()->appendRecipe(Expanded->getDefiningRecipe());
   }
----------------
nit: can cast instead of getting defined recipe.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2222
+  /// VPBasicBlock corresponding to the original preheader. Used to place
+  /// VPExpandSCEV recipes for expressions used during skeleton creation.
+  VPBasicBlock *Preheader;
----------------
nit: "... during skeleton creation and (rest of) VPlan execution."


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2271
+  /// Construct a VPlan with original preheader \p Preheader and  \p Entry to
+  /// the plan.
+  VPlan(VPBasicBlock *Preheader, VPBasicBlock *Entry)
----------------
nit: can add that (currently) they are disconnected because bypass blocks between them are not (yet) modelled in VPlan.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2267
+        VPValue *TripCount = nullptr)
+      : Entry(Entry), Preheader(Preheader), TripCount(TripCount) {
     if (Entry)
----------------
fhahn wrote:
> Ayal wrote:
> > nits: Entry >> VecPreheader? Should Preheader and TripCount drop their defaults and asserted to be non-null?
> Updated to require all arguments. I also added a version that doesn't take any arguments as this is used across different unit tests.
Oh, would this empty constructor prevent the creation of TripCount at construction rather than setting it later?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll:16
+; CHECK-NEXT: ph:
+; CHECK-NEXT:   EMIT vp<%0> = EXPAND SCEV (1 smax (1 + (sext i8 %y to i32))<nsw>)
+; CHECK-NEXT: No successors
----------------
Is vp<%0> used, does it match Live-in VEC_TC noted above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147964