[PATCH] D115793: [VPlan] Create header & latch blocks for plan skeleton up front (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9290
+  VPBlockUtils::tryToMergeBlockIntoPredecessor(LatchVPBB);
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
Should this folding of the latch be done right after folding the empty last VPBB above, or better do so here after dumping Plan.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2421
+        dyn_cast_or_null<VPBasicBlock>(Block->getSinglePredecessor());
+    if (!VPBB || !PredVPBB || !PredVPBB->getSingleSuccessor())
+      return false;
----------------
`!PredVPBB->getSingleSuccessor()` - suffice to check if `PredVPBB->getNumSuccessors() != 1`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2427
+    VPBlockUtils::disconnectBlocks(PredVPBB, VPBB);
+    auto *ParentRegion = dyn_cast<VPRegionBlock>(Block->getParent());
+    if (ParentRegion->getExit() == Block)
----------------
Either cast (non-dyn) if Block must be in a Region, or check `if (ParentRegion && ParentRegion->getExit() == Block)`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2357
   /// has more than one successor, its conditional bit is propagated to \p
   /// NewBlock. \p NewBlock must have neither successors nor predecessors.
   static void insertBlockAfter(VPBlockBase *NewBlock, VPBlockBase *BlockPtr) {
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Would be good to update above documentation
> > > - condition bit is propagated? to NewBlock?
> > > - disconnects BlockPtr from all its successors and connects it with NewBlock as its successor
> > Should be updated, cond bits are now propagated (and removed from BlockPtr ) and moving successors is mentioned.
> Thanks!
> "If \p BlockPtr has more than one successor ..."?
Drop "If \p BlockPtr has more than one successor ..."? This method moves all successors of BlockPtr to be successors of NewBlock, also when this involves a single successor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793