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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9290
+  VPBlockUtils::tryToMergeBlockIntoPredecessor(LatchVPBB);
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
Ayal wrote:
> Should this folding of the latch be done right after folding the empty last VPBB above, or better do so here after dumping Plan.
I don't think so. I think main motivation is to have the latch/exit block separately so sink-after & other transforms have do not have to explicitly updated the exit block.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2427
+    VPBlockUtils::disconnectBlocks(PredVPBB, VPBB);
+    auto *ParentRegion = dyn_cast<VPRegionBlock>(Block->getParent());
+    if (ParentRegion->getExit() == Block)
----------------
Ayal wrote:
> Either cast (non-dyn) if Block must be in a Region, or check `if (ParentRegion && ParentRegion->getExit() == Block)`
For now, it should be safe to use `cast` directly, Updated, thanks!


================
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:
> 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.
Removed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793