[PATCH] D34798: [Dominators] Add CFGBuilder testing utility

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Ah, hmm - I was assuming this was an API for building the CFG data structures. But I see it's an API for building IR that represents particular CFGs (so you can then test the CFG analysis that builds the data structures, etc, I assume?).

How much more terse is this than the hand-written IR? (I assume that's the motivation/benefit?) How much does that help - perhaps you could show (not in anything that needs to be committed) an example of the textual IR for some of the tests, to demonstrate how much more terse it is? But I guess it's obvious enough perhaps from the tabular update array format that it is quite compact.



================
Comment at: unittests/IR/CFGBuilder.cpp:30
+
+CFGHolder::~CFGHolder() {}
+
----------------
"= default", and maybe a comment about why this is out of line (I'm assuming it's out of line/not implicit so that the header doesn't need to include the Module header, etc and can use forward declarations instead)


================
Comment at: unittests/IR/CFGBuilder.cpp:44
+  auto *IntTy =
+      IntegerType::get(From->getParent()->getParent()->getContext(), 32);
+
----------------
I think you can rephrase

  From->getParent()->getParent()->getContext()

to

  From->getContext()

(also BB->getParent()->getParent() is equivalent to BB->getModule() I think?)


================
Comment at: unittests/IR/CFGBuilder.h:59-61
+    bool operator<(const Arc &RHS) const {
+      return std::make_pair(From, To) < std::make_pair(RHS.From, RHS.To);
+    }
----------------
Generally operator overloads that can be non-members should be (ensures equal conversion handling for the operands/arguments)


================
Comment at: unittests/IR/CFGBuilder.h:60
+    bool operator<(const Arc &RHS) const {
+      return std::make_pair(From, To) < std::make_pair(RHS.From, RHS.To);
+    }
----------------
probably use std::tie rather than std::make_pair, so that copies aren't needed.


================
Comment at: unittests/IR/CFGBuilder.h:66
+  struct Update {
+    Op Action;
+    Arc Arc;
----------------
It feels like the type name may be better to have a more meaningful name - rather than or in addition to the variable name (Operation? "Operation Op" ?)


================
Comment at: unittests/IR/CFGBuilder.h:67
+    Op Action;
+    Arc Arc;
+  };
----------------
Variable and type names being the same can be tricky - but I realize sometimes it's the tidiest option. *shrug* :)


================
Comment at: unittests/IR/CFGBuilder.h:70-71
+
+  using ArcListTy = std::vector<Arc>;
+  using UpdateListTy = std::vector<Update>;
+  CFGBuilder(Function *F, const ArcListTy &InitialArcs, UpdateListTy Updates);
----------------
Not sure if the aliases aid in readability (especially using the word 'list' in the names when they are vectors may given a mistaken impression about the performance of certain operations) - consider using the type names directly.


https://reviews.llvm.org/D34798