[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:9115
       }
     }
   }
----------------
Another alternative to FillHeaderVPBB above, is to prepare here for the next iteration by doing:

```
  auto *NextVPBB = new VPBasicBlock();
  VPBlockUtils::insertBlockAfter(NextVPBB, VPBB);
  VPBB = NextVPBB;
```
The start of each iteration needs to only `VPBB->setName(BB->getName())` which also takes care of the header block; after exiting the loop we can get rid of the last empty VPBB, possibly as part of fusing the latch.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9291
+  }
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
Would be good to avoid further bloating this excessive method. Could this folding be outlined, or should it be applied only to selected VPlan prior to code-gen rather than every VPlan upon construction? Doesn't VPBasicBlock::execute() effectively fold Exit into its predecessor if possible?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9045
+    else
+      FirstIter = false;
     VPBB = FirstVPBBForBB;
----------------
fhahn wrote:
> 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.
> (Always generating a new VPBB and ...
On second thought, header phi recipes should be placed in the header VPBB, which may be awkward if current "VPBB" moved on.
Another alternative is mentioned below, which always generates the next new VPBB.


================
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) {
----------------
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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2360
     assert(NewBlock->getSuccessors().empty() &&
            "Can't insert new block with successors.");
+    NewBlock->setParent(BlockPtr->getParent());
----------------
Also assert NewBlock has no predecessors, as documented above?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2368
     BlockPtr->setOneSuccessor(NewBlock);
     NewBlock->setPredecessors({BlockPtr});
   }
----------------
While we're here, connectBlocks(BlockPtr, NewBlock); ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2370
+    SmallVector<VPBlockBase *> Succs(BlockPtr->getSuccessors().begin(),
+                                     BlockPtr->getSuccessors().end());
+    for (VPBlockBase *Succ : Succs) {
----------------
fhahn wrote:
> 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.
Very well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793