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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:97
+      VPBBPreds.push_back(PredRegion);
+      assert(VPBB == cast<VPBasicBlock>(PredRegion->getSingleSuccessor()));
+    }
----------------
Ayal wrote:
> 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.
> 
Reordered assert, added message, thanks!


================
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
----------------
Ayal wrote:
> Comment needs updating.
Updated, thanks!


================
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;
----------------
Ayal wrote:
> 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.
Adjusted to check the parent region, thanks!


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:170
   if (CurrentLoop) {
     auto Iter = Loop2Region.insert({CurrentLoop, nullptr});
     if (Iter.second)
----------------
Ayal wrote:
> 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});
>   }
> ```
adjusted, thanks!


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:319
   VPBasicBlock *ThePreheaderVPBB = Plan.getEntry();
   BB2VPBB[ThePreheaderBB] = ThePreheaderVPBB;
   ThePreheaderVPBB->setName("vector.ph");
----------------
Ayal wrote:
> buildPlain/HierarchicalCFG() is called following VPlan::createInitialVPlan(), which creates VPBB's (and will create more, plus Region, in D158333) directly rather than via getOrCreateVPBB(); so need to revisit them and update BB2VPBB, Loop2Region, setParent. Worth a comment.
Added a comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:330
+  ThePreheaderVPBB->setOneSuccessor(TopRegion);
+  TopRegion->getEntry()->setName("vector.body");
 
----------------
Ayal wrote:
> Simpler to do something like
> 
> ```
>   VPBlockBase *HeaderVPBB = getOrCreateVPBB(TheLoop->getHeader());
>   HeaderVPBB->setName("vector.body");
>   ThePreheaderVPBB->setOneSuccessor(HeaderVPBB->getParent());
> 
> ```
> ?
Simplified, thanks!


================
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);
----------------
Ayal wrote:
> 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().
Flipped, thanks!


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


================
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
----------------
Ayal wrote:
> "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?
Updated to us LI to check for latch and use early `continue`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:385
+      VPB->setOneSuccessor(Successors[0]);
+    else if (Successors.size() == 2)
+      VPB->setTwoSuccessors(Successors[0], Successors[1]);
----------------
Ayal wrote:
> else { 
> 
> Already asserted above that numSuccs is 1 or 2.
Adjusted, thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136