[PATCH] D48815: [VPlan] Introduce VPlan-based dominator analysis.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:198
+  return std::distance(df_iterator<const VPBlockBase *>::begin(Entry),
+                       df_iterator<const VPBlockBase *>::end(Exit));
+}
----------------
dcaballe wrote:
> I'd like to hear your feedback on this. Internally, we started with an 'unsigned size' class member that we tried to maintain transformation after transformation. However, there are many cases where a simple size update is not possible and you need some kind of CFG traversal (sometimes a full region's traversal, such as in this code). For example, when you wrap part of a CFG with a region, when you insert or remove part of a CFG into/from a region, etc. Since 'getSize' is only used by GraphTraits and it's not a highly used interface, I though it could be cleaner to just compute the size when needed instead of trying to store and maintain it. However, if you think this is an overkill, we could give it a try to the latter approach.
It looks like is is not used for building a dominator tree. Maybe we should only add once it's needed?

Storing  the size and updating it in the transformations would be better IMO and with an assertion here that checks the stored size matches the computed size it should be easy to track down cases where it is not updated properly.



================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:592
+using VPDomTree = DomTreeBase<VPBlockBase>;
+template void DomTreeBuilder::Calculate<VPDomTree>(VPDomTree &DT);
----------------
Use VPDominatorTree defined in VPlanDominatorTree.h?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1319
+template <>
+struct GraphTraits<const VPRegionBlock *>
+    : public GraphTraits<const VPBlockBase *> {
----------------
The only difference to the non-const VPRegionBlock* is the type parameter, right? It's not a lot of code, but would it make sense to add a templated implementation which we then can instantiate with both <VPRegionBlock*> and <const VPRegionBlock*>?




================
Comment at: lib/Transforms/Vectorize/VPlan.h:1340
+template <>
+struct GraphTraits<Inverse<VPRegionBlock *>>
+    : public GraphTraits<Inverse<VPBlockBase *>> {
----------------
If we implement GraphTraits<const VPRegionBlock*>, should we also implement to const variant here?


================
Comment at: lib/Transforms/Vectorize/VPlanDominatorTree.h:27
+/// VPBlockBases.
+class VPDominatorTree : public DomTreeBase<VPBlockBase> {
+public:
----------------
can we just use `using VPDominatorTree = DomTreeBase<VPBlockBase>;` here?


================
Comment at: lib/Transforms/Vectorize/VPlanDominatorTree.h:34
+/// VPBlockBases.
+class VPPostDominatorTree : public PostDomTreeBase<VPBlockBase> {
+public:
----------------
same as for VPDominatorTree


================
Comment at: unittests/Transforms/Vectorize/VPlanDominatorTreeTest.cpp:59
+  // Reachability.
+  EXPECT_TRUE(VPDT.isReachableFromEntry(PH));
+  EXPECT_TRUE(VPDT.isReachableFromEntry(H));
----------------
I am wondering if we could use a mapping from IR BBs to VPBBs and just match if the DT and VPDT agree on all cases. So instead testing everything manually, we rely on the DT to be correct.


https://reviews.llvm.org/D48815