[PATCH] D140513: [VPlan] Switch default graph traits to be recursive, update VPDomTree.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 20 07:36:52 PST 2023
fhahn marked 4 inline comments as done.
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:451
- using ParentPtrTy = VPRegionBlock *;
+ using ParentPtrTy = VPlan *;
----------------
Ayal wrote:
> 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?
This has been removed in the latest version based on D142162
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:62
public:
+ using reference = BlockPtrTy;
+
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > who's using this `reference`?
> > It is used by `iterator_facade_base` with `bidirectional_iterator_tag`
> OK, worth a comment?
Added, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:261
+ using NodeRef = VPBlockBase *;
+ using ChildIteratorType = SmallVectorImpl<VPBlockBase *>::iterator;
+
----------------
Ayal wrote:
> 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.
I missed uploading the part, should be fixed. in the latest version.
================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:62
+ // / \ |--|
+ // R1BB2 R1BB3 -|
+ // \ /
----------------
Ayal wrote:
> 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.
Sounds good, I can prepare a follow-up.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140513/new/
https://reviews.llvm.org/D140513
More information about the llvm-commits
mailing list