[PATCH] D159136: [VPlan] Update successors/predecessors of region blocks directly (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:143
+
+static bool isHeaderVPBB(VPBasicBlock *VPBB) {
+  return VPBB->getParent() && VPBB->getParent()->getEntry() == VPBB;
----------------
isHeaderVPBB used only in assert, define under !NDEBUG?
Suggest below to reuse it outside of asserts.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:162
     // Retrieve existing VPBB.
     return BlockIt->second;
   }
----------------
nit: BlockIt is not needed below, can simply check

```
    // Retrieve VPBB if exists.
    if (BB2VPBB.contains(BB))
      return BB2VPBB[BB];
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:171
+  // Get or create a region for the loop containing BB.
+  Loop *LoopOfBB = LI->getLoopFor(BB);
+  VPRegionBlock *RegionOfVPBB = nullptr;
----------------
nit: can early exit here via

```
  if (!LoopOfBB)
    return VPBB;
```
and define RegionOfVPBB inside then/else when setting it.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:174
+  if (LoopOfBB) {
+    bool RegionExists = Loop2Region.contains(LoopOfBB);
+    if (RegionExists) {
----------------
nit: assert BB-is-header xor RegionExists?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:180
+      RegionOfVPBB = new VPRegionBlock(LoopOfBB->getHeader()->getName().str(),
+                                       false /*isReplicator*/);
+      RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
----------------
`      VPBB->setParent(RegionOfVPBB);`
here too?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:181
+                                       false /*isReplicator*/);
+      RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
+      RegionOfVPBB->setEntry(VPBB);
----------------
When called with BB being the header of TheLoop, its parentLoop may be null or not; in any case, will Loop2Region[TheLoop->getParentLoop()] surely return null?
Can set the parent of RegionOfVPBB only if LoopOfBB != TheLoop.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:183
+      RegionOfVPBB->setEntry(VPBB);
+      Loop2Region.insert({LoopOfBB, RegionOfVPBB});
+    }
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:348
+    // Set VPBB predecessors in the same order as they are in the incoming BB.
+    if (isHeaderBB(BB, LoopForBB)) {
+      // BB is a loop header, set the predecessor for the region.
----------------
nit: can handle the simpler non-header case first.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:372
+    // successor of the region.
+    if (LoopForBB && BB == LoopForBB->getLoopLatch()) {
+      assert(Successors.size() == 2 && "latch must have 2 successors");
----------------
Trying to sketch how this could be done using only getOrCreateVPBB(), i.e., w/o getOrCreateVPB(), ordered according to simplicity:

```
    unsigned NumSuccs = succ_size(BB);
    if (NumSuccs == 1) {
      auto *Successor = getOrCreateVPBB(BB->getSingleSuccessor());
      VPBB->setOneSuccessor(isHeaderVPBB(Successor) ? Successor->getParent()
                                                    : Successor);
      continue;
    }

    assert(NumSuccs == 2 && "block must have one or two successors");
    // Look up the branch condition to get the corresponding VPValue
    // representing the condition bit in VPlan (which may be in another VPBB).
    assert(IRDef2VPValue.contains(BI->getCondition()) &&
           "Missing condition bit in IRDef2VPValue!");
    VPBasicBlock *Successor0 = getOrCreateVPBB(BI->getSuccessor(0));
    VPBasicBlock *Successor1 = getOrCreateVPBB(BI->getSuccessor(1));
    if (!isLatchBB(BB, LoopForBB)) {
      VPBB->setTwoSuccessors(Successor0, Successor1);
      continue;
    }
    // For a latch we need to set the successor of the region rather than that
    // of VPBB and it should be set to the exit, i.e., non-header successor.
    Region->setOneSuccessor(isHeaderVPBB(Successor0) ? Successor1 : Successor0);
    Region->setExiting(VPBB);

```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:377
+      Region->setExiting(VPBB);
+      Region->setParent(ExitVPBB->getParent());
+      // For the latch, we need to set the successor for the region.
----------------
Should the parents of Region and ExitVPBB already be set, presumably to the same parent?
(As will be asserted when setting one to be the successor of the other.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136