[Support][NFC] Simplify DomTreeNodeBase::addChild

[aengelke]

Previously, the method would take the unique_ptr, store the pointer in its Children vector, and then return the unique_ptr. Pass the raw pointer as parameter instead.

This was added in a72d6ef when introducing unique_ptr, previosuly this was a source code size optimization.

---

Context: this will simplify replacing DomTreeNodes with a different data structure, see this RFC.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Alexis Engelke (aengelke)

Changes

Previously, the method would take the unique_ptr, store the pointer in its Children vector, and then return the unique_ptr. Pass the raw pointer as parameter instead.

This was added in a72d6ef when introducing unique_ptr, previosuly this was a source code size optimization.

---

Context: this will simplify replacing DomTreeNodes with a different data structure, see this RFC.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/Support/GenericDomTree.h (+7-10)

diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index a8e178d6461e0..5bf4a34eb9770 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -90,11 +90,7 @@ template <class NodeT> class DomTreeNodeBase {
   DomTreeNodeBase *getIDom() const { return IDom; }
   unsigned getLevel() const { return Level; }
 
-  std::unique_ptr<DomTreeNodeBase> addChild(
-      std::unique_ptr<DomTreeNodeBase> C) {
-    Children.push_back(C.get());
-    return C;
-  }
+  void addChild(DomTreeNodeBase *C) { Children.push_back(C); }
 
   bool isLeaf() const { return Children.empty(); }
   size_t getNumChildren() const { return Children.size(); }
@@ -655,8 +651,8 @@ class DominatorTreeBase {
     } else {
       assert(Roots.size() == 1);
       NodeT *OldRoot = Roots.front();
-      auto &OldNode = DomTreeNodes[OldRoot];
-      OldNode = NewNode->addChild(std::move(DomTreeNodes[OldRoot]));
+      DomTreeNodeBase<NodeT> *OldNode = getNode(OldRoot);
+      NewNode->addChild(OldNode);
       OldNode->IDom = NewNode;
       OldNode->UpdateLevel();
       Roots[0] = BB;
@@ -831,9 +827,10 @@ class DominatorTreeBase {
   void addRoot(NodeT *BB) { this->Roots.push_back(BB); }
 
   DomTreeNodeBase<NodeT> *createChild(NodeT *BB, DomTreeNodeBase<NodeT> *IDom) {
-    return (DomTreeNodes[BB] = IDom->addChild(
-                std::make_unique<DomTreeNodeBase<NodeT>>(BB, IDom)))
-        .get();
+    auto Node = std::make_unique<DomTreeNodeBase<NodeT>>(BB, IDom);
+    auto *NodePtr = (DomTreeNodes[BB] = std::move(Node)).get();
+    IDom->addChild(NodePtr);
+    return NodePtr;
   }
 
   DomTreeNodeBase<NodeT> *createNode(NodeT *BB) {

[nikic]

Can we do something like this?

Suggested change

	auto *NodePtr = (DomTreeNodes[BB] = std::move(Node)).get();
	IDom->addChild(NodePtr);
	return NodePtr;
	auto *NodePtr = Node.get();
	DomTreeNodes[BB] = std::move(Node);
	return NodePtr;

Really not a fan of this "using return value of move assignment operator" pattern...

[kuhar]

+1

[aengelke]

Changed. (I didn't like that either, but wanted at least some consistency with createNode below.) I now merged this method with createNode to avoid code duplication.

[nikic]

LGTM