From e80799e6afc6959d61e086080ec490df397ee122 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Wed, 23 Jan 2019 22:59:52 +0000 Subject: [PATCH] [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 --- llvm/include/llvm/ADT/ilist.h | 9 +++---- llvm/include/llvm/CodeGen/MachineFunction.h | 2 +- llvm/lib/CodeGen/MachineBasicBlock.cpp | 8 ++++-- llvm/lib/IR/SymbolTableListTraitsImpl.h | 3 ++- llvm/unittests/ADT/IListTest.cpp | 30 +++++++++++++++++++++ 5 files changed, 43 insertions(+), 9 deletions(-) diff --git a/llvm/include/llvm/ADT/ilist.h b/llvm/include/llvm/ADT/ilist.h index 62e434010ddf..06c7abff965f 100644 --- a/llvm/include/llvm/ADT/ilist.h +++ b/llvm/include/llvm/ADT/ilist.h @@ -65,9 +65,8 @@ template 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 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 a3b13fb2438a..670b2d501af3 100644 --- a/llvm/include/llvm/CodeGen/MachineFunction.h +++ b/llvm/include/llvm/CodeGen/MachineFunction.h @@ -85,7 +85,7 @@ template <> struct ilist_callback_traits { template 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 df4762088c1d..c3e9c185be9a 100644 --- a/llvm/lib/CodeGen/MachineBasicBlock.cpp +++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp @@ -132,8 +132,12 @@ void ilist_traits::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 a74ac8a960f9..f399c823d6fb 100644 --- a/llvm/lib/IR/SymbolTableListTraitsImpl.h +++ b/llvm/lib/IR/SymbolTableListTraitsImpl.h @@ -83,7 +83,8 @@ void SymbolTableListTraits::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 3992a2354035..18f6c41a6480 100644 --- a/llvm/unittests/ADT/IListTest.cpp +++ b/llvm/unittests/ADT/IListTest.cpp @@ -207,6 +207,12 @@ struct NodeWithCallback : ilist_node { } // 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 { + static void deleteNode(NodeWithCallback *) {} +}; + template <> struct ilist_callback_traits { 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 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 { friend struct llvm::ilist_detail::NodeAccess;