[PATCH] D140513: [VPlan] Switch default graph traits to be recursive, update VPDomTree.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 18 05:21:48 PST 2023
Ayal added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:451
- using ParentPtrTy = VPRegionBlock *;
+ using ParentPtrTy = VPlan *;
----------------
who's using ParentPtrTy? Parent itself is still a VPRegionBase*...
================
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; }
----------------
Is the above FIXME related to the current changes which include `front` below, and/or a good time to address?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:62
public:
+ using reference = BlockPtrTy;
+
----------------
who's using this `reference`?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:261
+ using NodeRef = VPBlockBase *;
+ using ChildIteratorType = SmallVectorImpl<VPBlockBase *>::iterator;
+
----------------
Should this inverse use something like VPAllPredecessorsIterator?
Would be good to test the order of an inverse iterator.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:272
+ }
+};
+
----------------
GraphTraits for Regions removed as no longer needed?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanCFG.h:277
+ using NodeRef = VPBlockBase *;
+ // using ChildIteratorType = SmallVectorImpl<VPBlockBase *>::iterator;
+ using nodes_iterator = df_iterator<NodeRef>;
----------------
commented-out code to be removed?
================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:62
+ // / \ |--|
+ // R1BB2 R1BB3 -|
+ // \ /
----------------
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)
================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:71
+ // |
+ // R2BB2
+ //
----------------
nit: ` // }`
================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:110
+ EXPECT_FALSE(VPDT.dominates(R2BB1, R1BB1));
+
+ EXPECT_TRUE(VPDT.dominates(R1, R2BB1));
----------------
Also
`EXPECT_TRUE(VPDT.dominates(R1BB4, R2BB1));`
`EXPECT_FALSE(VPDT.dominates(R1BB3, R2BB1));`
?
================
Comment at: llvm/unittests/Transforms/Vectorize/VPDomTreeTest.cpp:160
+ VPBasicBlock *VPBB2 = new VPBasicBlock("VPBB2");
+ VPBlockUtils::connectBlocks(R1, VPBB2);
+ }
----------------
Here's a good place to build a dominator tree and check its edges?
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