[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:9025
+  // Create initial VPlan skeleton, with separate header and latch blocks.
+  VPBasicBlock *HeaderVPBB = new VPBasicBlock(OrigLoop->getHeader()->getName());
+  VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
----------------
Note that setting the name of HeaderVPBB here became redundant. So is setting next.vpbb below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9115
       }
     }
   }
----------------
fhahn wrote:
> Ayal wrote:
> > 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.
> Thanks, updated to use this approach, but with a small extra to avoid generating a redundant empty block in the last iteration.
> 
> Alternatively `VPBlockUtils::tryToMergeBlockIntoPredecessor` could be used, if you prefer.
Perhaps early-exit, combine with the bump, and count backwards:

```
  if (--NumBBsToProcess == 0)
    continue/break;
  ...
```
It seemed simpler to avoid the counting and checking by paying for a redundant empty block at the end; it indeed could be cleaned up using tryToMergeBlockIntoPredecessor (thanks for outlining!), although it needs only a disconnect and delete.
Pick whichever version you prefer.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9291
+  }
+
   assert(VPlanVerifier::verifyPlanIsValid(*Plan) && "VPlan is invalid");
----------------
fhahn wrote:
> Ayal wrote:
> > 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?
> I added a new helper `tryToMergeBlockIntoPredecessor`. 
> 
> > . 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?
> 
> The only reason to do this here is to avoid polluting the VPlan printing with additional redundant blocks. We could keep the extra block, but it would require updating all tests that check a printed VPlan and also bloats the plans we need to check in general. WDYT?
Ah, sure, let's clean up VPlans upon construction then.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2370
+    NewBlock->setCondBit(BlockPtr->getCondBit());
+    BlockPtr->setCondBit(nullptr);
+    connectBlocks(BlockPtr, NewBlock);
----------------
(These condition bits seem to be poorly tested...)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2419
+    auto *VPBB = dyn_cast<VPBasicBlock>(Block);
+    auto *PredVPBB = dyn_cast<VPBasicBlock>(Block->getSinglePredecessor());
+    if (!VPBB || !PredVPBB || !PredVPBB->getSingleSuccessor())
----------------
dyn_cast_or_null, or must Block have a single predecessor?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2429
+      if (TopRegion->getExit() == Block)
+        TopRegion->setExit(PredVPBB);
+    }
----------------
Update Block->getParent()'s Exit if it exists and is equal to 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) {
----------------
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 ..."?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793