[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.h:1319
+template <>
+struct GraphTraits<const VPRegionBlock *>
+    : public GraphTraits<const VPBlockBase *> {
----------------
fhahn wrote:
> dcaballe wrote:
> > 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 :)
> Thanks, I think either way is fine and I suppose simpler error messages trump the slightly more compact code :)
Ok, I'll revert it then.


================
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();
----------------
fhahn wrote:
> dcaballe wrote:
> > 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?
> A friend class could work, if a forward declaration is enough (I don't think we want to include the unit test file here). googletest also ships a FRIEND_TEST macro (https://github.com/google/googletest/blob/master/googletest/include/gtest/gtest_prod.h) and from that I think the name of the friend class should have the following format: `
> #define FRIEND_TEST(test_case_name, test_name)\
> friend class test_case_name##_##test_name##_Test`
Thanks a lot. I thought the friendship would be more problematic. Done!


================
Comment at: unittests/Transforms/Vectorize/VPlanDominatorTreeTest.cpp:59
+  // Reachability.
+  EXPECT_TRUE(VPDT.isReachableFromEntry(PH));
+  EXPECT_TRUE(VPDT.isReachableFromEntry(H));
----------------
fhahn wrote:
> dcaballe wrote:
> > 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? 
> I don't think it's worth spending too much time on that, I just thought it might be a slightly more compact way to write the checks (DTs are widely used and *heavily* tested :))
Ok, let's go with this. More than an exhaustive testing of VPDT, what I want to test here is the new GraphTraits code and this should be enough for that.


https://reviews.llvm.org/D48815