[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/include/llvm/IR/CFGDiff.h:199
+namespace {
+template <bool B> struct reverse_if_helper;
+template <> struct reverse_if_helper<true> {
----------------
You can use two helper functions taking `integral_constant<bool, true/false>` tags -- should be less verbose


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:89
     bool IsRecalculated = false;
+    GraphDiffT *PreViewOfCFG;
+    const size_t NumLegalized;
----------------
nit: Why pointer instead of keeping a reference?


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:177
       constexpr bool Direction = IsReverse != IsPostDom;  // XOR.
-      for (const NodePtr Succ :
-           ChildrenGetter<Direction>::Get(BB, BatchUpdates)) {
+      typedef
+          typename std::conditional<Direction, Inverse<NodePtr>, NodePtr>::type
----------------
nit: use `using` instead of `typefef`.


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:178
+      typedef
+          typename std::conditional<Direction, Inverse<NodePtr>, NodePtr>::type
+              IsRevType;
----------------
nit: can we use `std::conditional_t<...>` in c++14 to get rid of `typename` and `::type`? Or does it make some buildbots unhappy?


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:179
+          typename std::conditional<Direction, Inverse<NodePtr>, NodePtr>::type
+              IsRevType;
+      using GraphDiffBBPair = std::pair<const GraphDiffT *, IsRevType>;
----------------
nit: I don't find this name very informative. Maybe something like `DirectedBB`?


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:183
+          BatchUpdates ? BatchUpdates->PreViewOfCFG : EmptyGD.get();
+      std::pair<const GraphDiffT *, NodePtr> GDNodePair =
+          std::make_pair(GDTmp, BB);
----------------
`std::pair<const GraphDiffT *, NodePtr> GDNodePair(GDTmp, BB)`


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:185
+          std::make_pair(GDTmp, BB);
+      for (auto &Pair : children<GraphDiffBBPair>(GDNodePair)) {
+        const NodePtr Succ = Pair.second;
----------------
Won't children infer the template parameter based on the passes value? I don't get why the template argument type is a pair where the second argument is a directed nodeptr, but the runtime value is always a plain nodeptr. Couldn't the second pair type also be directed for `GDNodePair`?


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:186
+      for (auto &Pair : children<GraphDiffBBPair>(GDNodePair)) {
+        const NodePtr Succ = Pair.second;
         const auto SIT = NodeToInfo.find(Succ);
----------------
Could this new code be a hoisted into a helper function?


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:1146
       if (Update.getKind() == UpdateKind::Insert)
-        DT.insertEdge(Update.getFrom(), Update.getTo());
+        InsertEdge(DT, nullptr, Update.getFrom(), Update.getTo());
       else
----------------
Could you add a comment next to the nullptr with the argument name?


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:1543
+  // FIXME: Updated to use the PreViewCFG and behave the same as until now.
+  // This behavior is however incorrect; this actually needs the PostViewCFG.
+  GraphDiff<typename DomTreeT::NodePtr, DomTreeT::IsPostDominator> PreViewCFG(
----------------
Does this care about the direction (pre- or post-) at all, or does it need some CFG view? Why is pre-view incorrect?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77341/new/

https://reviews.llvm.org/D77341