[PATCH] D159136: [VPlan] Simplify HCFG construction of region blocks (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:85
 void PlainCFGBuilder::setVPBBPredsFromBB(VPBasicBlock *VPBB, BasicBlock *BB) {
   SmallVector<VPBlockBase *, 8> VPBBPreds;
   // Collect VPBB predecessors.
----------------
Ayal wrote:
> Perhaps the following would more clearly update the current code by handling the exit block case separately, than during traversal over all predecessors:
> ```
>   if (auto *LatchBB = getLatchOfExit(BB)) {
>     auto *PredRegion = getOrCreateVPBB(LatchBB)->getParent();
>     assert(VPBB == cast<VPBasicBlock>(PredRegion->getSingleSuccessor()) &&
>            "successor must already be set for PredRegion; it must have VPBB "
>            "as single successor");
>     VPBB->setPredecessors({PredRegion});
>     return;
>   }
>   // Collect VPBB predecessors.
>   SmallVector<VPBlockBase *, 2> VPBBPreds;
>   for (BasicBlock *Pred : predecessors(BB))
>     VPBBPreds.push_back(getOrCreateVPBB(Pred));
>   VPBB->setPredecessors(VPBBPreds);
> ```
> where a lambda or function would check isExitBB(), analogous to isHeaderBB(), possibly along with providing the Latch:
> 
> ```
> BasicBlock* getLatchOfExit(BasicBlock *BB) {
>   auto *SinglePred = BB->getSinglePredecessor();
>   if (!SinglePred || LI->getLoopFor(SinglePred) == LI->getLoopFor(BB))
>     return nullptr;
>   // can assert SinglePred is the latch of its loop.
>   return SinglePred;
> }
> 
> ```
> 
> ?
update, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:108
+  // BB is a loop header block. Connect the region to the loop preheader,
+  // identified by checking if the parent doesn't match Region.
+  VPBasicBlock *PreheaderVPBB = nullptr;
----------------
Ayal wrote:
> Worth augmenting the comment with a note that this loop not only searches for the preheader (for which it could early-break once found or employ find_if()) but also (gets or) creates VPBB's for (all) its latch(es).
> 
> This loop over predecessors has a trip count of 2; would unrolling it be clearer?
> 
> 
> ```
>   assert(pred_size(BB) == 2 && "A preheader and single latch are expected");
>   auto PredBBI = pred_begin(BB);
>   auto *PredVPBB0 = getOrCreateVPBB(*PredBBI);
>   auto *PredVPBB1 = getOrCreateVPBB(*++PredBBI);
>   int Pred0InRegion = PredVPBB0->getParent() == Region;
>   assert(Pred0InRegion ^ PredVPBB1->getParent() == Region &&
>          "Expecting latch to be in region but not preheader");
>   Region->setPredecessors({Pred0InRegion ? PredVPBB1 : PredVPBB0});
> 
> ```
Now that we liberally use loop info, we can just use that here as well to identify the loop predecessor.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:170-172
+  if (RegionExists) {
+    VPBB->setParent(Loop2Region[LoopOfBB]);
+  } else {
----------------
Ayal wrote:
> 
I think the current coding standard recommends using branches for both `if` and `else` when either requires them (https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements). Left the code as is for now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:369
+    VPBasicBlock *Successor1 = getOrCreateVPBB(BI->getSuccessor(1));
+    if (!LoopForBB || BB != LoopForBB->getLoopLatch()) {
+      VPBB->setTwoSuccessors(Successor0, Successor1);
----------------
Ayal wrote:
> Note that checking if `BB == LoopForBB->getLoopLatch()` implies a single latch, compared to checking if `LoopForBB->isLoopLatch(BB)`. This is fine, and independent of handling loops with multiple exiting blocks.
Ack!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:162
     // Retrieve existing VPBB.
     return BlockIt->second;
   }
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > nit: BlockIt is not needed below, can simply check
> > > 
> > > ```
> > >     // Retrieve VPBB if exists.
> > >     if (BB2VPBB.contains(BB))
> > >       return BB2VPBB[BB];
> > > ```
> > Yes, but that would require 2 lookups I think?
> Yes, but such 2 lookups should be folded by the compiler rather than the programmer...
> We do use contains()/[] below for Loop2Region.
> Would this work better:
> ```
>   // Retrieve VPBB if exists.
>   if (auto BlockIt = BB2VPBB.find(BB) != BB2VPBB.end())
>     return BlockIt->second;
> ```
> ?
> Yes, but such 2 lookups should be folded by the compiler rather than the programmer...
Agreed, but I think in this case the current compilers won't be able to do so :(

> Would this work better:



I don't think this will work, as BlockIt would be a bool I think (result of the compare).

Probably better to use `lookup`, which returns `nullptr` if the key doesn't exist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136