[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:97
+      VPBBPreds.push_back(PredRegion);
+      assert(VPBB == cast<VPBasicBlock>(PredRegion->getSingleSuccessor()));
+    }
----------------
This assumes/asserts BB is not a header block, with a preheader PredVPBB having VPBB's region as a single successor.

This assumes successors have already been set, before setting predecessors.

nit: assert first; add error message.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:106
+                                           BasicBlock *BB) {
+  // If VPBB is a region block, we are dealing with a loop header block. Connect
+  // the region to the loop preheader, identified by checking the number of
----------------
Comment needs updating.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:109
+  // successors (the latch won't have any successors yet, as it has not been
+  // processed at this time.
+  VPBasicBlock *PreheaderVPBB = nullptr;
----------------
Better to distinguish between latch and preheader by checking if enclosing regions are the same, as in setVPBBPredsFromBB(), rather than relying on having already set the successor(s) of preheader but not yet of the latch? The latch should have no successors even after processing it.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:168
   Loop *CurrentLoop = LI->getLoopFor(BB);
   VPRegionBlock *ParentR = nullptr;
   if (CurrentLoop) {
----------------
CurrentLoop >> LoopOfBB
ParentR >> RegionOfVPBB


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:170
   if (CurrentLoop) {
     auto Iter = Loop2Region.insert({CurrentLoop, nullptr});
     if (Iter.second)
----------------
May be clearer to do something like

```
  bool RegionExists = Loop2Region.contains(CurrentLoop);
  if (RegionExists)
    ParentR = Loop2Region[CurrentLoop];
  else {
    ParentR = new VPRegionBlock(...);
    ParentR->setEntry(VPBB); // Or feed it in the constructor.
    Loop2Region.insert({CurrentLoop, ParentR});
  }
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:178
+    if (CurrentLoop != TheLoop)
+      ParentR->setParent(Loop2Region[CurrentLoop->getParentLoop()]);
+    if (Iter.second)
----------------
nit: worth separating the two if's with an empty line.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:182
+    else
+      VPBB->setParent(ParentR);
   }
----------------
Should VPBB's parent be set to ParentR also for header blocks which create the region when visited?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:330
+  ThePreheaderVPBB->setOneSuccessor(TopRegion);
+  TopRegion->getEntry()->setName("vector.body");
 
----------------
Simpler to do something like

```
  VPBlockBase *HeaderVPBB = getOrCreateVPBB(TheLoop->getHeader());
  HeaderVPBB->setName("vector.body");
  ThePreheaderVPBB->setOneSuccessor(HeaderVPBB->getParent());

```
?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:345
+      // BB is a loop header, set the predecessor for the region.
+      assert(LI->getLoopFor(BB)->getHeader() == BB);
+      setRegionPredsFromBB(Region, BB);
----------------
Check if `BB` is header of its loop according to `LI`, and assert that Region's entry is VPBB, rather than the other way around? I.e., have isHeaderBB() rather than isHeaderVPBB().


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:363
+    assert(Successors.size() == 1 ||
+           IRDef2VPValue.count(cast<BranchInst>(TI)->getCondition()) &&
+               "Missing condition bit in IRDef2VPValue!");
----------------
nit: count >> contains


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:366
+
+    // If VPBB is the loop latch i.e. one of its predecessor is the region
+    // itself, update the region's exiting block and the successor of the
----------------
"one of its predecessors is the region" >> "one of its successors is the header, which is set to its region by getOrCreateVPB(), in constrast to getOrCreateVPBB()".

This is confusing.
Can we keep using getOrCreateVPBB() only, checking if `BB` is pre/header/latch using `LI`, and processing its (predecessors and) successors accordingly?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:385
+      VPB->setOneSuccessor(Successors[0]);
+    else if (Successors.size() == 2)
+      VPB->setTwoSuccessors(Successors[0], Successors[1]);
----------------
else { 

Already asserted above that numSuccs is 1 or 2.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:386
+    else if (Successors.size() == 2)
+      VPB->setTwoSuccessors(Successors[0], Successors[1]);
   }
----------------
Make sure first successor corresponds to True?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:349
              "Missing condition bit in IRDef2VPValue!");
 
+      VPRegionBlock *Region = VPBB->getParent();
----------------
fhahn wrote:
> Ayal wrote:
> > Worth dealing here with four cases explicitly: BB is header or not, BB is latch or not?
> > 
> > Setting predecessors: easier to check if BB is header of its loop, and if so hook the region of its VPBB (setting its entry) to a single predecessor - the VPBB of preheader - having a distinct Region? Otherwise setVPBBPredsFromBB.
> > 
> > Setting successors: easier to check if BB is latch of its loop, and if so hook the region of its VPBB (setting its exit) to a single successor - the VPBB of exit - having a distinct Region? Otherwise set VPBB's successor(s).
> Updated to handle header and latch separately while setting successors. To simplify the latter, I updated the code to collect the successors in a vector, as this is used to check for the latch.
Proposed above to check for latch using parental regions than number of successors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136