[Support][ADT] Minor cleanups after #101706

[aengelke]

Fix GraphHasNodeNumbers indicator for graphs where NodeRef is not the graph type (e.g., Inverse<...>).

Add static_assert tests to MachineBasicBlock GraphTraits.

Use GraphHasNodeNumbers in DomTreeBase.

Don't include ad-hoc numbering map for numbered graphs in DomTreeBase.

[llvmbot]

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-adt

Author: Alexis Engelke (aengelke)

Changes

Fix GraphHasNodeNumbers indicator for graphs where NodeRef is not the graph type (e.g., Inverse<...>).

Add static_assert tests to MachineBasicBlock GraphTraits.

Use GraphHasNodeNumbers in DomTreeBase.

Don't include ad-hoc numbering map for numbered graphs in DomTreeBase.

---

Full diff: https://github.com/llvm/llvm-project/pull/102180.diff

3 Files Affected:

• (modified) llvm/include/llvm/ADT/GraphTraits.h (+2-1)
• (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+12)
• (modified) llvm/include/llvm/Support/GenericDomTree.h (+14-14)

diff --git a/llvm/include/llvm/ADT/GraphTraits.h b/llvm/include/llvm/ADT/GraphTraits.h
index 0764ecb4bb568..20bb27f50e17f 100644
--- a/llvm/include/llvm/ADT/GraphTraits.h
+++ b/llvm/include/llvm/ADT/GraphTraits.h
@@ -97,7 +97,8 @@ struct GraphTraits {
 
 namespace detail {
 template <typename T>
-using has_number_t = decltype(GraphTraits<T>::getNumber(std::declval<T>()));
+using has_number_t = decltype(GraphTraits<T>::getNumber(
+    std::declval<typename GraphTraits<T>::NodeRef>()));
 } // namespace detail
 
 /// Indicate whether a GraphTraits<NodeT>::getNumber() is supported.
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 797e29d071dd9..2d238326ee1a3 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -1304,6 +1304,9 @@ template <> struct GraphTraits<MachineBasicBlock *> {
   }
 };
 
+static_assert(GraphHasNodeNumbers<MachineBasicBlock *>,
+              "GraphTraits getNumber() not detected");
+
 template <> struct GraphTraits<const MachineBasicBlock *> {
   using NodeRef = const MachineBasicBlock *;
   using ChildIteratorType = MachineBasicBlock::const_succ_iterator;
@@ -1318,6 +1321,9 @@ template <> struct GraphTraits<const MachineBasicBlock *> {
   }
 };
 
+static_assert(GraphHasNodeNumbers<const MachineBasicBlock *>,
+              "GraphTraits getNumber() not detected");
+
 // Provide specializations of GraphTraits to be able to treat a
 // MachineFunction as a graph of MachineBasicBlocks and to walk it
 // in inverse order.  Inverse order for a function is considered
@@ -1341,6 +1347,9 @@ template <> struct GraphTraits<Inverse<MachineBasicBlock*>> {
   }
 };
 
+static_assert(GraphHasNodeNumbers<Inverse<MachineBasicBlock *>>,
+              "GraphTraits getNumber() not detected");
+
 template <> struct GraphTraits<Inverse<const MachineBasicBlock*>> {
   using NodeRef = const MachineBasicBlock *;
   using ChildIteratorType = MachineBasicBlock::const_pred_iterator;
@@ -1358,6 +1367,9 @@ template <> struct GraphTraits<Inverse<const MachineBasicBlock*>> {
   }
 };
 
+static_assert(GraphHasNodeNumbers<Inverse<const MachineBasicBlock *>>,
+              "GraphTraits getNumber() not detected");
+
 // These accessors are handy for sharing templated code between IR and MIR.
 inline auto successors(const MachineBasicBlock *BB) { return BB->successors(); }
 inline auto predecessors(const MachineBasicBlock *BB) {
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index d7b94d50e6311..38f62e86ae652 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -262,7 +262,9 @@ class DominatorTreeBase {
       SmallVector<std::unique_ptr<DomTreeNodeBase<NodeT>>>;
   DomTreeNodeStorageTy DomTreeNodes;
   // For graphs where blocks don't have numbers, create a numbering here.
-  DenseMap<const NodeT *, unsigned> NodeNumberMap;
+  std::conditional_t<!GraphHasNodeNumbers<NodeT *>,
+                     DenseMap<const NodeT *, unsigned>, std::tuple<>>
+      NodeNumberMap;
   DomTreeNodeBase<NodeT> *RootNode = nullptr;
   ParentPtr Parent = nullptr;
 
@@ -355,12 +357,8 @@ class DominatorTreeBase {
   }
 
 private:
-  template <typename T>
-  using has_number_t =
-      decltype(GraphTraits<T *>::getNumber(std::declval<T *>()));
-
   std::optional<unsigned> getNodeIndex(const NodeT *BB) const {
-    if constexpr (is_detected<has_number_t, NodeT>::value) {
+    if constexpr (GraphHasNodeNumbers<NodeT *>) {
       // BB can be nullptr, map nullptr to index 0.
       assert(BlockNumberEpoch ==
                  GraphTraits<ParentPtr>::getNumberEpoch(Parent) &&
@@ -374,7 +372,7 @@ class DominatorTreeBase {
   }
 
   unsigned getNodeIndexForInsert(const NodeT *BB) {
-    if constexpr (is_detected<has_number_t, NodeT>::value) {
+    if constexpr (GraphHasNodeNumbers<NodeT *>) {
       // getNodeIndex will never fail if nodes have getNumber().
       unsigned Idx = *getNodeIndex(BB);
       if (Idx >= DomTreeNodes.size()) {
@@ -736,7 +734,8 @@ class DominatorTreeBase {
     }
 
     DomTreeNodes[*IdxOpt] = nullptr;
-    NodeNumberMap.erase(BB);
+    if constexpr (!GraphHasNodeNumbers<NodeT *>)
+      NodeNumberMap.erase(BB);
 
     if (!IsPostDom) return;
 
@@ -830,7 +829,7 @@ class DominatorTreeBase {
 private:
   void updateBlockNumberEpoch() {
     // Nothing to do for graphs that don't number their blocks.
-    if constexpr (is_detected<has_number_t, NodeT>::value)
+    if constexpr (GraphHasNodeNumbers<NodeT *>)
       BlockNumberEpoch = GraphTraits<ParentPtr>::getNumberEpoch(Parent);
   }
 
@@ -849,9 +848,8 @@ class DominatorTreeBase {
   }
 
   /// Update dominator tree after renumbering blocks.
-  template <class T_ = NodeT>
-  std::enable_if_t<is_detected<has_number_t, T_>::value, void>
-  updateBlockNumbers() {
+  template <typename T = NodeT>
+  std::enable_if_t<GraphHasNodeNumbers<T *>, void> updateBlockNumbers() {
     updateBlockNumberEpoch();
 
     unsigned MaxNumber = GraphTraits<ParentPtr>::getMaxNumber(Parent);
@@ -889,7 +887,8 @@ class DominatorTreeBase {
 
   void reset() {
     DomTreeNodes.clear();
-    NodeNumberMap.clear();
+    if constexpr (!GraphHasNodeNumbers<NodeT *>)
+      NodeNumberMap.clear();
     Roots.clear();
     RootNode = nullptr;
     Parent = nullptr;
@@ -989,7 +988,8 @@ class DominatorTreeBase {
   /// assignable and destroyable state, but otherwise invalid.
   void wipe() {
     DomTreeNodes.clear();
-    NodeNumberMap.clear();
+    if constexpr (!GraphHasNodeNumbers<NodeT *>)
+      NodeNumberMap.clear();
     RootNode = nullptr;
     Parent = nullptr;
   }

[kuhar]

nit: consider adding todo to switch to an empty struct with no_unique_address once we update to C++20.

[kuhar]

Looks good overall