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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:451
 
-  using ParentPtrTy = VPRegionBlock *;
+  using ParentPtrTy = VPlan *;
 
----------------
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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2172
   // GraphTraints::getEntryNode. We should add a new template parameter to
   // DominatorTreeBase representing the Graph type.
   VPBlockBase &front() const { return *Entry; }
----------------
Ayal wrote:
> Is the above FIXME related to the current changes which include `front` below, and/or a good time to address?
Yes, it might be good to address this FIXME now. I'll look into it.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:261
+  using NodeRef = VPBlockBase *;
+  using ChildIteratorType = SmallVectorImpl<VPBlockBase *>::iterator;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:272
+  }
+};
+
----------------
Ayal wrote:
> GraphTraits for Regions removed as no longer needed?
Yep


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:277
+  using NodeRef = VPBlockBase *;
+  // using ChildIteratorType = SmallVectorImpl<VPBlockBase *>::iterator;
+  using nodes_iterator = df_iterator<NodeRef>;
----------------
Ayal wrote:
> commented-out code to be removed?
removed, thanks!


================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:62
+    //    /     \   |--|
+    //  R1BB2   R1BB3 -|
+    //    \      /
----------------
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?


================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:71
+    //      |
+    //    R2BB2
+    //
----------------
Ayal wrote:
> nit: `    //  }`
Updated, thanks!


================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:110
+    EXPECT_FALSE(VPDT.dominates(R2BB1, R1BB1));
+
+    EXPECT_TRUE(VPDT.dominates(R1, R2BB1));
----------------
Ayal wrote:
> Also
> `EXPECT_TRUE(VPDT.dominates(R1BB4, R2BB1));`
> `EXPECT_FALSE(VPDT.dominates(R1BB3, R2BB1));`
> ?
added, thanks!


================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:160
+    VPBasicBlock *VPBB2 = new VPBasicBlock("VPBB2");
+    VPBlockUtils::connectBlocks(R1, VPBB2);
+  }
----------------
Ayal wrote:
> Here's a good place to build a dominator tree and check its edges?
Updated code here to check the immediate dominators, which is more compact. I retained the dominance checks above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140513