[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:162
     // Retrieve existing VPBB.
     return BlockIt->second;
   }
----------------
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?


================
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;
----------------
Ayal wrote:
> nit: can early exit here via
> 
> ```
>   if (!LoopOfBB)
>     return VPBB;
> ```
> and define RegionOfVPBB inside then/else when setting it.
Adjusted, thanks!


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:181
+                                       false /*isReplicator*/);
+      RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
+      RegionOfVPBB->setEntry(VPBB);
----------------
Ayal wrote:
> 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.
It should be guaranteed to be null for `TheLoop->getParentLoop()`, as no entries for it will be added.


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


================
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.
----------------
Ayal wrote:
> nit: can handle the simpler non-header case first.
reordered, thanks!


================
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");
----------------
Ayal wrote:
> 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);
> 
> ```
Adjusted, thanks!


================
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.
----------------
Ayal wrote:
> 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.)
Yep, parent of the region is already set, removed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136