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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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);
 
----------------
Ayal wrote:
> Can be made const?
Thanks, I adjusted the comment, made it const and moved the trivial definition here as well.


================
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);
----------------
Ayal wrote:
> Passing InsertBlock is now redundant?
Yep, argument has been removed.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3281
    scalar remainder.
 
        [ ] <-- loop iteration number check.
----------------
Ayal wrote:
> Worth adding old preheader to the drawing, now that SCEVs are expanded there?
They'll get expanded in the same block that has the loop iteration check. I updated the comment.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7707
+  BestVPlan.getEntry()->execute(&State);
+
   Value *CanonicalIVStartValue;
----------------
Ayal wrote:
> Easier to set here `ILV.TripCount = State.get(BestVPlan.getTripCount(), 0);` than pass Plan and State to createVectorizedLoopSkeleton() just for that?
Adjusted, thanks! Unfortunately this requires removing the recipes in the preheader block before executing the plan for the vector epilogue; it has to re-use the expanded values from the main vector loop.


================
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");
----------------
Ayal wrote:
> Above comment should also include the new (old) pre-header.
Extended the comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8925
+  VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
+  VPBlockUtils::insertBlockAfter(VecPreheader, Preheader);
 
----------------
Ayal wrote:
> 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.
I updated the code to have the preheader separate (pre-header before skeleton creation). I *think* at the moment the skeleton creation code leaves the CFG in a valid state, so for now SCEV expansions only needed by the vector body can remain where they are at the moment.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9130
+      new VPExpandSCEVRecipe(ExitCount, *PSE.getSE());
+  Preheader->appendRecipe(TripCount);
+
----------------
Ayal wrote:
> 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.
moved the code to add the trip count right after plan construction.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9155
                         CM.getTailFoldingStyle());
+
   return Plan;
----------------
Ayal wrote:
> nit: empty line intentional?
Removed, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2292
     if (!TripCount)
       TripCount = new VPValue();
     return TripCount;
----------------
Ayal wrote:
> 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?
replaced with an assert


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147964