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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9115
       }
     }
   }
----------------
Ayal wrote:
> 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.
updated to just fold the block afterwards.


================
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");
----------------
Ayal wrote:
> Note that setting the name of HeaderVPBB here became redundant. So is setting next.vpbb below.
I removed it for HeaderVPBB both.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2370
+    NewBlock->setCondBit(BlockPtr->getCondBit());
+    BlockPtr->setCondBit(nullptr);
+    connectBlocks(BlockPtr, NewBlock);
----------------
Ayal wrote:
> (These condition bits seem to be poorly tested...)
yes, they are only used in the native path and it looks like the function is not used on blocks with condbits. Not sure if we can really improve that.


================
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())
----------------
Ayal wrote:
> dyn_cast_or_null, or must Block have a single predecessor?
updated to dyn_cast_or_null.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2429
+      if (TopRegion->getExit() == Block)
+        TopRegion->setExit(PredVPBB);
+    }
----------------
Ayal wrote:
> Update Block->getParent()'s Exit if it exists and is equal to Block?
updated, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115793