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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8798
   VPBlockUtils::insertBlockAfter(RegSucc, Region);
+  VPBlockUtils::connectBlocks(RegSucc, SingleSucc);
   return RegSucc;
----------------
Ayal wrote:
> Perhaps something like VPBlockUtils::insertBlockOnEdge() would help take care of ensuring single successor, disconnecting, reconnecting? Admittedly two blocks are being inserted here.
I left it as is for now, because multiple blocks are added and connected here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9022
+  VPBasicBlock *HeaderVPBB = new VPBasicBlock(OrigLoop->getHeader()->getName());
+  auto *TopRegion = new VPRegionBlock("vector loop");
+  TopRegion->setEntry(HeaderVPBB);
----------------
Ayal wrote:
> Feed Entry and Exit to constructor of VPRegionBlock instead of setting them explicitly?
> Feed Entry to constructor of VPlan instead of setting it explicitly?
Thanks, updated!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9045
+    else
+      FirstIter = false;
     VPBB = FirstVPBBForBB;
----------------
Ayal wrote:
> Can be simplified into:
> 
>   if (FillHeaderVPBB)
>     FillHeaderVPBB = false;
>   else {
>     auto *FirstVPBBForBB = new VPBasicBlock(BB->getName());
>     VPBlockUtils::insertBlockAfter(FirstVPBBForBB, VPBB);
>     VPBB = FirstVPBBForBB;
>   }
> 
> ?
> 
> (Always generating a new VPBB and fusing the empty "dummy" header later, along with fusing latch below, would be reverting D111299?)
Simplified,thanks!

> (Always generating a new VPBB and fusing the empty "dummy" header later, along with fusing latch below, would be reverting D111299?)

Perhaps partly reverting it. But in the current patch the header VPBB is actually used, whereas pre-D111299 this 'dummy pre-entry' was not used for anything.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2370
+    SmallVector<VPBlockBase *> Succs(BlockPtr->getSuccessors().begin(),
+                                     BlockPtr->getSuccessors().end());
+    for (VPBlockBase *Succ : Succs) {
----------------
Ayal wrote:
> Something early_inc_range could handle?
Unfortunately I don't think `early_inc_range` will work here, because the successors are managed in a SmallVector, so the early incremented iterator may be invalid after `disconnectBlocks` removes an entry from the vector. (It *might* work when reversing the iteration order and early increment, but it seems to me that using a SMallVector here is safer for now.

I added a `VPBlockBase::successors()` helper that returns an iterator range, to avoid the ugly SmallVector construction. This makes some existing use also nicer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793