[ADT] Notify ilist traits about in-list transfers

[rnk]

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.

[chandlerc]

I live in fear of the use cases we haven't yet thought of for this that we will discover in the future. ;] But proceed, LGTM!

Maybe add a unittest exercising this so we don't break it accidentally and fail to notice? No need for another round of review there.

[rnk]

• add a unit test

[rnk]

In D57120#1368541, @chandlerc wrote:

I live in fear of the use cases we haven't yet thought of for this that we will discover in the future. ;] But proceed, LGTM!

Yeah, this is a pretty intimate specialization point for a data structure to have. But, it's not widely used outside of LLVM's core IR (and MIR) data structures.

Maybe add a unittest exercising this so we don't break it accidentally and fail to notice? No need for another round of review there.

Done, thanks!

llvm/trunk/include/llvm/ADT/ilist.h

 /// Callbacks do nothing by default in iplist and ilist.
 ///
 /// Specialize this for to use callbacks for when nodes change their list
 /// membership.
 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*/) {
     (void)OldList;
   }
 };
 
 /// A fragment for template traits for intrusive list that provides default

 private:
   // transfer - The heart of the splice function.  Move linked list nodes from
   // [first, last) into position.
   //
   void transfer(iterator position, iplist_impl &L2, iterator first, iterator last) {
     if (position == last)
       return;
 
-    if (this != &L2) // Notify traits we moved the nodes...
-      this->transferNodesFromList(L2, first, last);
+    // Notify traits we moved the nodes...
 
     base_list_type::splice(position, L2, first, last);
   }
 
 public:
   //===----------------------------------------------------------------------===
   // Functionality derived from other functions defined above...
   //

llvm/trunk/include/llvm/CodeGen/MachineFunction.h

 };
 
 template <> struct ilist_callback_traits<MachineBasicBlock> {
   void addNodeToList(MachineBasicBlock* N);
   void removeNodeFromList(MachineBasicBlock* N);
 
   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");
   }
 };
 
 /// MachineFunctionInfo - This class can be derived from and used by targets to
 /// hold private target-specific information for each MachineFunction.  Objects
 /// of type are accessed/created with MF::getInfo and destroyed when the
 /// MachineFunction is destroyed.
 struct MachineFunctionInfo {

llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp

 }
 
 /// When moving a range of instructions from one MBB list to another, we need to
 /// update the parent pointers and the use/def lists.
 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
   // parent pointers.
   for (; First != Last; ++First)
     First->setParent(Parent);
 }
 

llvm/trunk/lib/IR/SymbolTableListTraitsImpl.h

if (ValueSymbolTable *ST = getSymTab(getListOwner()))
       ST->removeValueName(V->getValueName());
 }
 
 template <typename ValueSubClass>
 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...
   ValueSymbolTable *NewST = getSymTab(NewIP);
   ValueSymbolTable *OldST = getSymTab(OldIP);
   if (NewST != OldST) {
     for (; first != last; ++first) {
       ValueSubClass &V = *first;

llvm/trunk/unittests/ADT/IListTest.cpp

struct NodeWithCallback : ilist_node<NodeWithCallback> {
   NodeWithCallback() = default;
   NodeWithCallback(int Value) : Value(Value) {}
   NodeWithCallback(const NodeWithCallback &) = delete;
 };
 
 } // 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; }
   template <class Iterator>
   void transferNodesFromList(ilist_callback_traits &Other, Iterator First,
                              Iterator Last) {
     for (; First != Last; ++First) {
       First->WasTransferred = true;
TEST(IListTest, addNodeToList) {
   ASSERT_TRUE(N.WasTransferred);
 
   L1.remove(&N);
   ASSERT_EQ(0u, L1.size());
   ASSERT_FALSE(N.IsInList);
   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;
 
   int Value = 0;
 
   PrivateNode() = default;
   PrivateNode(int Value) : Value(Value) {}
   PrivateNode(const PrivateNode &) = delete;