[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.h:1319
+template <>
+struct GraphTraits<const VPRegionBlock *>
+    : public GraphTraits<const VPBlockBase *> {
----------------
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 :)


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1340
+template <>
+struct GraphTraits<Inverse<VPRegionBlock *>>
+    : public GraphTraits<Inverse<VPBlockBase *>> {
----------------
dcaballe wrote:
> 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 *>
Sounds good to me!


================
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();
----------------
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`


================
Comment at: unittests/Transforms/Vectorize/VPlanDominatorTreeTest.cpp:59
+  // Reachability.
+  EXPECT_TRUE(VPDT.isReachableFromEntry(PH));
+  EXPECT_TRUE(VPDT.isReachableFromEntry(H));
----------------
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 :))


https://reviews.llvm.org/D48815