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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks for accommodating! Adding a couple of last minor comments.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:89
+      return nullptr;
+    // can assert SinglePred is the latch of its loop.
+    return SinglePred;
----------------
This comment should be replaced by an assert, or dropped.

Note (somewhere) that exit blocks are assumed to have a single predecessor.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:175
+
+  bool RegionExists = Loop2Region.contains(LoopOfBB);
+  assert(RegionExists ^ isHeaderBB(BB, LoopOfBB) &&
----------------
Use lookup here as well instead of contains and []?
The default value constructed in case of absence would be a nullptr, given that value type is VPRegionBlock*.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:182
+    auto *RegionOfVPBB = new VPRegionBlock(
+        LoopOfBB->getHeader()->getName().str(), false /*isReplicator*/);
+    RegionOfVPBB->setParent(Loop2Region[LoopOfBB->getParentLoop()]);
----------------
Worth a comment, something like:
// If BB's loop is nested inside another loop within VPlan's scope, the header of that enclosing loop was already visited and its region constructed and recorded in Loop2Region. That region is now set as the parent of VPBB's region. Otherwise it is set to null.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:349-351
+    if (!isHeaderBB(BB, LoopForBB))
+      setVPBBPredsFromBB(VPBB, BB);
+    else {
----------------
Very well, let's be consistent.


================
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;
----------------
fhahn wrote:
> Ayal wrote:
> > 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});
> > 
> > ```
> Now that we liberally use loop info, we can just use that here as well to identify the loop predecessor.
Using LoopOfBB->getLoopPredecessor() is indeed much simpler!
Note that the latch VPBB will now be created (potentially) later when visiting its predecessor rather than here when visiting its successor (the header).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:162
     // Retrieve existing VPBB.
     return BlockIt->second;
   }
----------------
fhahn wrote:
> Ayal wrote:
> > 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;
> > ```
> > ?
> > Yes, but such 2 lookups should be folded by the compiler rather than the programmer...
> Agreed, but I think in this case the current compilers won't be able to do so :(
> 
> > Would this work better:
> 
> 
> 
> I don't think this will work, as BlockIt would be a bool I think (result of the compare).
> 
> Probably better to use `lookup`, which returns `nullptr` if the key doesn't exist.
Yes, lookup looks fine!
The default value constructed in case of absence would be a nullptr, given that value type is VPBasicBlock*.
(A parenthesis might make BlockIt the desired iterator.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159136