[llvm] [Support][ADT] Minor cleanups after #101706 (PR #102180)

[Alexis Engelke via llvm-commits]

https://github.com/aengelke updated https://github.com/llvm/llvm-project/pull/102180

>From 14f21b6faa2a8db82c581fcde54310e0c6f6e323 Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Tue, 6 Aug 2024 16:50:21 +0000
Subject: [PATCH 1/2] [Support][ADT] Minor cleanups after #101706

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.
---
 llvm/include/llvm/ADT/GraphTraits.h           |  3 +-
 llvm/include/llvm/CodeGen/MachineBasicBlock.h | 12 ++++++++
 llvm/include/llvm/Support/GenericDomTree.h    | 28 +++++++++----------
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/llvm/include/llvm/ADT/GraphTraits.h b/llvm/include/llvm/ADT/GraphTraits.h
index 0764ecb4bb5682..20bb27f50e17f9 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 797e29d071dd9a..2d238326ee1a30 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 d7b94d50e63111..38f62e86ae6524 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;
   }

>From 964d5e38ef2080e3be25c332e0a857aa41994147 Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Thu, 8 Aug 2024 12:44:27 +0000
Subject: [PATCH 2/2] Add remark on empty struct

---
 llvm/include/llvm/Support/GenericDomTree.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index 38f62e86ae6524..7e2b68e6faea29 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -262,6 +262,7 @@ class DominatorTreeBase {
       SmallVector<std::unique_ptr<DomTreeNodeBase<NodeT>>>;
   DomTreeNodeStorageTy DomTreeNodes;
   // For graphs where blocks don't have numbers, create a numbering here.
+  // TODO: use an empty struct with [[no_unique_address]] in C++20.
   std::conditional_t<!GraphHasNodeNumbers<NodeT *>,
                      DenseMap<const NodeT *, unsigned>, std::tuple<>>
       NodeNumberMap;