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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Raises some thoughts...
These ExpandSCEV recipes, which must expand into IR that can/must be placed in the original pre-header, before making any changes to the CFG, are conceptually Live-In's for which code needs to be generated, i.e., have a recipe, albeit a rather unique one, which can reside in a VPBB, albeit a rather unique one.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:502
   /// epilogue vectorization, this function is overriden to handle the more
   /// complex control flow around the loops.
+  virtual std::pair<BasicBlock *, Value *>
----------------
Worth mentioning \p Plan and State?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:611
 
   /// Returns (and creates if needed) the original loop trip count.
+  Value *getTripCount(BasicBlock *InsertBlock);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:612
   /// Returns (and creates if needed) the original loop trip count.
-  Value *getOrCreateTripCount(BasicBlock *InsertBlock);
+  Value *getTripCount(BasicBlock *InsertBlock);
 
----------------
Can be made const?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2893
 
-Value *InnerLoopVectorizer::getOrCreateTripCount(BasicBlock *InsertBlock) {
-  if (TripCount)
-    return TripCount;
-
-  assert(InsertBlock);
-  IRBuilder<> Builder(InsertBlock->getTerminator());
-  // Find the loop boundaries.
-  Type *IdxTy = Legal->getWidestInductionType();
-  assert(IdxTy && "No type for induction");
-  const SCEV *ExitCount = createTripCountSCEV(IdxTy, PSE);
-
-  const DataLayout &DL = InsertBlock->getModule()->getDataLayout();
-
-  // Expand the trip count and place the new instructions in the preheader.
-  // Notice that the pre-header does not change, only the loop body.
-  SCEVExpander Exp(*PSE.getSE(), DL, "induction");
-
-  // Count holds the overall loop count (N).
-  TripCount = Exp.expandCodeFor(ExitCount, ExitCount->getType(),
-                                InsertBlock->getTerminator());
-
-  if (TripCount->getType()->isPointerTy())
-    TripCount =
-        CastInst::CreatePointerCast(TripCount, IdxTy, "exitcount.ptrcnt.to.int",
-                                    InsertBlock->getTerminator());
-
+Value *InnerLoopVectorizer::getTripCount(BasicBlock *InsertBlock) {
+  assert(TripCount);
----------------
Passing InsertBlock is now redundant?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3281
    scalar remainder.
 
        [ ] <-- loop iteration number check.
----------------
Worth adding old preheader to the drawing, now that SCEVs are expanded there?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7706
+  State.Builder.SetInsertPoint(OrigLoop->getLoopPreheader()->getTerminator());
+  BestVPlan.getEntry()->execute(&State);
+
----------------
Better placed under "// 0. Generate SCEV-dependent code into the preheader, including TripCount, before making any changes to the CFG." ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7707
+  BestVPlan.getEntry()->execute(&State);
+
   Value *CanonicalIVStartValue;
----------------
Easier to set here `ILV.TripCount = State.get(BestVPlan.getTripCount(), 0);` than pass Plan and State to createVectorizedLoopSkeleton() just for that?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8914
   // followed by a region for the vector loop, followed by the middle block. The
   // skeleton vector loop region contains a header and latch block.
+  VPBasicBlock *Preheader = new VPBasicBlock("ph");
----------------
Above comment should also include the new (old) pre-header.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8925
+  VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
+  VPBlockUtils::insertBlockAfter(VecPreheader, Preheader);
 
----------------
Should Preheader be detached from VecPreheader (and any other block), given that they represent disparate blocks and also get executed separately? Retaining the latter as Plan's entryBlock, keeping the former as Plan's (currently) isolated block for this and all other ExpandSCEV recipes.
Perhaps worth verifying that all ExpandSCEV recipes reside in this unique Preheader VPBB.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9130
+      new VPExpandSCEVRecipe(ExitCount, *PSE.getSE());
+  Preheader->appendRecipe(TripCount);
+
----------------
nit: can make Plan here after creating TripCount and Preheader as when building with recipes instead of setTripCount() and setEntry(). HCFGBuilder will then take care of filling (Old)Entry, independently.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9155
                         CM.getTailFoldingStyle());
+
   return Plan;
----------------
nit: empty line intentional?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2292
     if (!TripCount)
       TripCount = new VPValue();
     return TripCount;
----------------
Is this code dead (and method can indeed drop its OrCreate and become const) - provided either a non-null argument was provided when constructing the VPlan with recipes or non-null TripCount was set when constructing w/o? Otherwise is TripCount deleted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147964