[ADT] Notify ilist traits about in-list transfers

Summary:
Previously no client of ilist traits has needed to know about transfers
of nodes within the same list, so as an optimization, ilist doesn't call
transferNodesFromList in that case. However, now there are clients that
want to use ilist traits to cache instruction ordering information to
optimize dominance queries of instructions in the same basic block.
This change updates the existing ilist traits users to detect in-list
transfers and do nothing in that case.

After this change, we can start caching instruction ordering information
in LLVM IR data structures. There are two main ways to do that:
- by putting an order integer into the Instruction class
- by maintaining order integers in a hash table on BasicBlock

I plan to implement and measure both, but I wanted to commit this change
first to enable other out of tree ilist clients to implement this
optimization as well.

Reviewers: lattner, hfinkel, chandlerc

Subscribers: hiraditya, dexonsmith, llvm-commits

Differential Revision: https://reviews.llvm.org/D57120

llvm-svn: 351992

diff --git a/llvm/include/llvm/ADT/ilist.h b/llvm/include/llvm/ADT/ilist.h
index 62e434010ddf6d07a8d92d929135e4edd77e20e5..06c7abff965f619e71a031e804b0d6a8a1fb3dce 100644 (file)
--- a/llvm/include/llvm/ADT/ilist.h
+++ b/llvm/include/llvm/ADT/ilist.h
@@ -65,9 +65,8 @@ template <typename NodeTy> struct ilist_callback_traits {
   void addNodeToList(NodeTy *) {}
   void removeNodeFromList(NodeTy *) {}
 
-  /// Callback before transferring nodes to this list.
-  ///
-  /// \pre \c this!=&OldList
+  /// Callback before transferring nodes to this list. The nodes may already be
+  /// in this same list.
   template <class Iterator>
   void transferNodesFromList(ilist_callback_traits &OldList, Iterator /*first*/,
                              Iterator /*last*/) {
@@ -286,8 +285,8 @@ private:
     if (position == last)
       return;
 
-    if (this != &L2) // Notify traits we moved the nodes...
-      this->transferNodesFromList(L2, first, last);
+    // Notify traits we moved the nodes...
+    this->transferNodesFromList(L2, first, last);
 
     base_list_type::splice(position, L2, first, last);
   }

diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h
index a3b13fb2438ad9423c3b9df0d364888bd3f0cc73..670b2d501af342e38829a7fb745b4b6c62e129ea 100644 (file)
--- a/llvm/include/llvm/CodeGen/MachineFunction.h
+++ b/llvm/include/llvm/CodeGen/MachineFunction.h
@@ -85,7 +85,7 @@ template <> struct ilist_callback_traits<MachineBasicBlock> {
 
   template <class Iterator>
   void transferNodesFromList(ilist_callback_traits &OldList, Iterator, Iterator) {
-    llvm_unreachable("Never transfer between lists");
+    assert(this == &OldList && "never transfer MBBs between functions");
   }
 };
 

diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp
index df4762088c1dbb1c083e780f53851b32949a2be2..c3e9c185be9a4f1a9a09021f8efbdbf71e82fdf5 100644 (file)
--- a/llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -132,8 +132,12 @@ void ilist_traits<MachineInstr>::transferNodesFromList(ilist_traits &FromList,
                                                        instr_iterator First,
                                                        instr_iterator Last) {
   assert(Parent->getParent() == FromList.Parent->getParent() &&
-        "MachineInstr parent mismatch!");
-  assert(this != &FromList && "Called without a real transfer...");
+         "cannot transfer MachineInstrs between MachineFunctions");
+
+  // If it's within the same BB, there's nothing to do.
+  if (this == &FromList)
+    return;
+
   assert(Parent != FromList.Parent && "Two lists have the same parent?");
 
   // If splicing between two blocks within the same function, just update the

diff --git a/llvm/lib/IR/SymbolTableListTraitsImpl.h b/llvm/lib/IR/SymbolTableListTraitsImpl.h
index a74ac8a960f933915250a0e16ef5bfc6cdaa10cd..f399c823d6fb0bf7638fc0bd30f9bcf398352493 100644 (file)
--- a/llvm/lib/IR/SymbolTableListTraitsImpl.h
+++ b/llvm/lib/IR/SymbolTableListTraitsImpl.h
@@ -83,7 +83,8 @@ void SymbolTableListTraits<ValueSubClass>::transferNodesFromList(
     SymbolTableListTraits &L2, iterator first, iterator last) {
   // We only have to do work here if transferring instructions between BBs
   ItemParentClass *NewIP = getListOwner(), *OldIP = L2.getListOwner();
-  assert(NewIP != OldIP && "Expected different list owners");
+  if (NewIP == OldIP)
+    return;
 
   // We only have to update symbol table entries if we are transferring the
   // instructions to a different symtab object...

diff --git a/llvm/unittests/ADT/IListTest.cpp b/llvm/unittests/ADT/IListTest.cpp
index 3992a2354035d9e0cd7e9cd97db672f455488444..18f6c41a64807636d32bd9613631db5dbd646583 100644 (file)
--- a/llvm/unittests/ADT/IListTest.cpp
+++ b/llvm/unittests/ADT/IListTest.cpp
@@ -207,6 +207,12 @@ struct NodeWithCallback : ilist_node<NodeWithCallback> {
 } // end namespace
 
 namespace llvm {
+// These nodes are stack-allocated for testing purposes, so don't let the ilist
+// own or delete them.
+template <> struct ilist_alloc_traits<NodeWithCallback> {
+  static void deleteNode(NodeWithCallback *) {}
+};
+
 template <> struct ilist_callback_traits<NodeWithCallback> {
   void addNodeToList(NodeWithCallback *N) { N->IsInList = true; }
   void removeNodeFromList(NodeWithCallback *N) { N->IsInList = false; }
@@ -247,6 +253,30 @@ TEST(IListTest, addNodeToList) {
   ASSERT_TRUE(N.WasTransferred);
 }
 
+TEST(IListTest, sameListSplice) {
+  NodeWithCallback N1(1);
+  NodeWithCallback N2(2);
+  ASSERT_FALSE(N1.WasTransferred);
+  ASSERT_FALSE(N2.WasTransferred);
+
+  ilist<NodeWithCallback> L1;
+  L1.insert(L1.end(), &N1);
+  L1.insert(L1.end(), &N2);
+  ASSERT_EQ(2u, L1.size());
+  ASSERT_EQ(&N1, &L1.front());
+  ASSERT_FALSE(N1.WasTransferred);
+  ASSERT_FALSE(N2.WasTransferred);
+
+  // Swap the nodes with splice inside the same list. Check that we get the
+  // transfer callback.
+  L1.splice(L1.begin(), L1, std::next(L1.begin()), L1.end());
+  ASSERT_EQ(2u, L1.size());
+  ASSERT_EQ(&N1, &L1.back());
+  ASSERT_EQ(&N2, &L1.front());
+  ASSERT_FALSE(N1.WasTransferred);
+  ASSERT_TRUE(N2.WasTransferred);
+}
+
 struct PrivateNode : private ilist_node<PrivateNode> {
   friend struct llvm::ilist_detail::NodeAccess;