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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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.
----------------
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;
}

```

?


================
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;
----------------
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});

```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:170-172
+  if (RegionExists) {
+    VPBB->setParent(Loop2Region[LoopOfBB]);
+  } else {
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:369
+    VPBasicBlock *Successor1 = getOrCreateVPBB(BI->getSuccessor(1));
+    if (!LoopForBB || BB != LoopForBB->getLoopLatch()) {
+      VPBB->setTwoSuccessors(Successor0, Successor1);
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:162
     // Retrieve existing VPBB.
     return BlockIt->second;
   }
----------------
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;
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136