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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:592
+using VPDomTree = DomTreeBase<VPBlockBase>;
+template void DomTreeBuilder::Calculate<VPDomTree>(VPDomTree &DT);
----------------
fhahn wrote:
> Use VPDominatorTree defined in VPlanDominatorTree.h?
OK. However, this only compiles if VPlanDominatorTree is a typedef. If VPDominatorTree turns into a derived class in the future, this won't compile (trying to use a protected class member). Hopefully, it will be easy to deduce that the error is coming from here.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:198
+  return std::distance(df_iterator<const VPBlockBase *>::begin(Entry),
+                       df_iterator<const VPBlockBase *>::end(Exit));
+}
----------------
fhahn wrote:
> 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.
> 
> It looks like is is not used for building a dominator tree. Maybe we should only add once it's needed?

Weird... now I can't find the patch that needed this code. Hopefully, this code is no longer 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.

OK. if needed, let's try but I would leave the assertion for VPlanVerifier. We may want to incrementally update the size even when the region is inconsistent (e.g., region's entry and exit are not reachable from each other). We may also need a 'recomputeSize' for those cases where we cannot do an incremental update and a full recomputation is needed.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1319
+template <>
+struct GraphTraits<const VPRegionBlock *>
+    : public GraphTraits<const VPBlockBase *> {
----------------
fhahn wrote:
> 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*>?
> 
> 
Well, lines 1299 and 1329 are explicit template specializations. I found this way to do it by using an extra level of inheritance but I'm not sure if the proposed solutions is worth the complexity. Deciphering an error message on these classes could be more challenging that desired :)


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1340
+template <>
+struct GraphTraits<Inverse<VPRegionBlock *>>
+    : public GraphTraits<Inverse<VPBlockBase *>> {
----------------
fhahn wrote:
> If we implement GraphTraits<const VPRegionBlock*>, should we also implement to const variant here?
We are introducing GraphTraits on demand and the const variant of Inverse<VPRegionBlock*> doesn't seem to be needed at this point. Same happens for Inverse<VPBasicBlock *>


================
Comment at: lib/Transforms/Vectorize/VPlanDominatorTree.h:27
+/// VPBlockBases.
+class VPDominatorTree : public DomTreeBase<VPBlockBase> {
+public:
----------------
fhahn wrote:
> can we just use `using VPDominatorTree = DomTreeBase<VPBlockBase>;` here?
Ok, we can turn it into a class when specific extensions are needed.


================
Comment at: lib/Transforms/Vectorize/VPlanDominatorTree.h:34
+/// VPBlockBases.
+class VPPostDominatorTree : public PostDomTreeBase<VPBlockBase> {
+public:
----------------
fhahn wrote:
> same as for VPDominatorTree
Sorry, I'll remove VPPostDominatorTree since it's not being used currently.


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.h:64
+  /// This interface is mainly used for unittests. To build the proper VPlan
+  /// H-CFG, 'buildHierarchicalCFG' must be used instead.
+  VPRegionBlock *buildPlainCFG();
----------------
I'm not too happy with making this interface public just for unittesting. Are there any examples of unittest for private member functions? Could friendship be applied here in a clean way?


================
Comment at: unittests/Transforms/Vectorize/VPlanDominatorTreeTest.cpp:59
+  // Reachability.
+  EXPECT_TRUE(VPDT.isReachableFromEntry(PH));
+  EXPECT_TRUE(VPDT.isReachableFromEntry(H));
----------------
fhahn wrote:
> 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.
Let me think about it but I'm not sure.  Wouldn't that hide bugs in the main algorithm that could be caught otherwise here? 


https://reviews.llvm.org/D48815