[PATCH] D158333: [VPlan] Move initial skeleton construction to createInitialVPlan. (NFC)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:723
+  auto *TopRegion = new VPRegionBlock("vector loop", false /*isReplicator*/);
+  VPBlockUtils::insertBlockAfter(TopRegion, Plan->getEntry());
+  VPBasicBlock *MiddleVPBB = new VPBasicBlock("middle.block");
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2544
+  /// which contains SCEV expansions that need to happen before the CFG is
+  /// modified; a VPbasicBlock for the vector pre-header, followed by a region
+  /// for the vector loop, followed by the middle VPBasicBlock.
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:307
+  // 0. Reuse the top-level region, vector-preheader and exit VPBBs from the
+  // skeleton.
+  VPRegionBlock *TheRegion = Plan.getVectorLoopRegion();
----------------
// These were created directly rather than via getOrCreateVPBB(), revisit them now to update BB2VPBB and Loop2region.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:313
+         "Unexpected loop preheader");
+  VPBasicBlock *VectorPreheaderVPBB = Plan.getEntry();
+  BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
----------------
nit:
`  auto *VectorPreheaderVPBB = cast<VPBasicBlock>(TheRegion->getSinglePredecessor());`
would be more consistent with how middle VPBB is retrieved.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:314
+  VPBasicBlock *VectorPreheaderVPBB = Plan.getEntry();
+  BB2VPBB[ThePreheaderBB] = VectorPreheaderVPBB;
+  BasicBlock *LoopExitBB = TheLoop->getUniqueExitBlock();
----------------
Worth noting that `ThePreheaderBB` conceptually corresponds to both Plan.getPreheader() (which wraps the original preheader BB) and Plan.getEntry() (which represents the new vector preheader); here we're interested in setting BB2VPBB to the latter.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:328
   // LoopBlocksDFS.
-  BasicBlock *ThePreheaderBB = TheLoop->getLoopPreheader();
-  assert((ThePreheaderBB->getTerminator()->getNumSuccessors() == 1) &&
-         "Unexpected loop preheader");
-  VPBasicBlock *ThePreheaderVPBB = Plan.getEntry();
-  BB2VPBB[ThePreheaderBB] = ThePreheaderVPBB;
-  ThePreheaderVPBB->setName("vector.ph");
+  VectorPreheaderVPBB->setName("vector.ph");
   for (auto &I : *ThePreheaderBB) {
----------------
Is this redundant - createInitialVPlan() already sets the name of VectorPreheaderVPBB to "vector.ph" when creating it?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:337
+  HeaderVPBB->setName("vector.body");
+  HeaderVPBB->setParent(TheRegion);
+  TheRegion->setEntry(HeaderVPBB);
----------------
Is setting the parent of HeaderVPBB to TheRegion redundant - getOrCreateVPBB() should have done so already?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:338
+  HeaderVPBB->setParent(TheRegion);
+  TheRegion->setEntry(HeaderVPBB);
 
----------------
Should this setEntry be done along with all others?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:354
       assert(LI->getLoopFor(BB)->getHeader() == BB);
-      setRegionPredsFromBB(Region, BB);
+      if (TheRegion != Region)
+        setRegionPredsFromBB(Region, BB);
----------------
nit: may be simpler to handle the non-header case first, then potentially else-if the header-of-non-TheRegion case.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:384
           Successors[0] == Region ? Successors[1] : Successors[0];
       Region->setExiting(VPBB);
       Region->setParent(ExitVPBB->getParent());
----------------
If Exits of regions are set here when identifying latches during successor setting, should Entries be set above when identifying headers during predecessor setting?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:386
       Region->setParent(ExitVPBB->getParent());
-      Successors = {ExitVPBB};
+      if (TheRegion != Region)
+        Successors = {ExitVPBB};
----------------
nit: continue if TheRegion == Region?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158333