[PATCH] D140513: [VPlan] Switch default graph traits to be recursive, update VPDomTree.

[Ayal Zaks via Phabricator via llvm-commits]

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

Looks good to me, with a few minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:451
 
-  using ParentPtrTy = VPRegionBlock *;
+  using ParentPtrTy = VPlan *;
 
----------------
fhahn wrote:
> Ayal wrote:
> > who's using ParentPtrTy? Parent itself is still a VPRegionBase*...
> This is a workaround for the issue that the dominator tree base classes rely on `NodeT::getParent()` to return a container type that all nodes share, like `Function` for basic blocks.
> 
> But for VPBlockBase it returns the parent region which may be nullptr. To have a DT spanning all regions in a plan, the parent needs to be `VPlan`.
> 
> There probably are other options to address this, D141260 is just a relatively simple/non-invasive way to get things to build and work. Now that we seem to be converging on cross region DTs, we should iron out the workaround as well
OK, worth leaving behind a comment?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:62
 public:
+  using reference = BlockPtrTy;
+
----------------
fhahn wrote:
> Ayal wrote:
> > who's using this `reference`?
> It is used by `iterator_facade_base` with `bidirectional_iterator_tag`
OK, worth a comment?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:261
+  using NodeRef = VPBlockBase *;
+  using ChildIteratorType = SmallVectorImpl<VPBlockBase *>::iterator;
+
----------------
fhahn wrote:
> Ayal wrote:
> > Should this inverse use something like VPAllPredecessorsIterator?
> > 
> > Would be good to test the order of an inverse iterator.
> I think the inverse is only needed for DT updates and post-dominators. We don't use either at the moment, I updated all implementations here to use `llvm_unreachable` and added a TODO. But it should not be needed for DT functionality we need to get started.
Very well.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:61
+  for (VPBlockBase *C : ExpectedChildren)
+    ExpectedNodes.push_back(VPDT.getNode(C));
+
----------------
nit: this also checks the children order, in addition to dominance relation.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:62
+    //    /     \   |--|
+    //  R1BB2   R1BB3 -|
+    //    \      /
----------------
fhahn wrote:
> Ayal wrote:
> > nit: ascii art at the ends of the two lines above which represent a cycle, may look better using
> > 
> > ```
> >      __
> >  \  /  \
> > R1BB3   |
> >  /  \__/
> > ```
> > where all incoming edges appear on top and all outgoing edges appear on bottom.
> > 
> > (A cycle deserves a region, here and below. When all loop back arcs are captured inside loop regions the HCFG is a (hierarchical) DAG)
> Improved the ASCII art, thanks!
> 
> > (A cycle deserves a region, here and below. When all loop back arcs are captured inside loop regions the HCFG is a (hierarchical) DAG)
> 
> The intention of the test is to make sure cycles in the graph are handled. We could forbid explicit cycles in the verifier though?
Currently cycles should be captured in regions so that, e.g., VPRegionBlock::execute() would allocate loops, and this is arguably the approach going forward. So it would probably be better to forbid explicit cycles for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140513