Sometimes (rarely) nodes held in LegalizeTypes
authorDuncan Sands <baldrick@free.fr>
Wed, 11 Jun 2008 11:42:12 +0000 (11:42 +0000)
committerDuncan Sands <baldrick@free.fr>
Wed, 11 Jun 2008 11:42:12 +0000 (11:42 +0000)
maps can be deleted.  This happens when RAUW
replaces a node N with another equivalent node
E, deleting the first node.  Solve this by
adding (N, E) to ReplacedNodes, which is already
used to remap nodes to replacements.  This means
that deleted nodes are being allowed in maps,
which can be delicate: the memory may be reused
for a new node which might get confused with the
old deleted node pointer hanging around in the
maps, so detect this and flush out maps if it
occurs (ExpungeNode).  The expunging operation
is expensive, however it never occurs during
a llvm-gcc bootstrap or anywhere in the nightly
testsuite.  It occurs three times in "make check":
Alpha/illegal-element-type.ll,
PowerPC/illegal-element-type.ll and
X86/mmx-shift.ll.  If expunging proves to be too
expensive then there are other more complicated
ways of solving the problem.
In the normal case this patch adds the overhead
of a few more map lookups, which is hopefully
negligable.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@52214 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/CodeGen/DAGISelHeader.h
include/llvm/CodeGen/SelectionDAG.h
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
lib/CodeGen/SelectionDAG/LegalizeTypes.h
lib/CodeGen/SelectionDAG/SelectionDAG.cpp

index 3188cc271acdd65a767b97c64b73e9d6cf937c07..3ac9affb357437c5889530587ab56388844c1d4e 100644 (file)
@@ -95,14 +95,14 @@ class VISIBILITY_HIDDEN ISelQueueUpdater :
       : ISelQueue(isq), HadDelete(false) {}
     
     bool hadDelete() const { return HadDelete; }
-    
+
     /// NodeDeleted - remove node from the selection queue.
-    virtual void NodeDeleted(SDNode *N) {
+    virtual void NodeDeleted(SDNode *N, SDNode *E) {
       ISelQueue.erase(std::remove(ISelQueue.begin(), ISelQueue.end(), N),
                       ISelQueue.end());
       HadDelete = true;
     }
-    
+
     /// NodeUpdated - Ignore updates for now.
     virtual void NodeUpdated(SDNode *N) {}
   };
index 20b54c29a34135a115dc7d2b5942bdfb45022c94..13a2920683739812149a5f80df689bd0cec62d7a 100644 (file)
@@ -485,7 +485,12 @@ public:
   class DAGUpdateListener {
   public:
     virtual ~DAGUpdateListener();
-    virtual void NodeDeleted(SDNode *N) = 0;
+
+    /// NodeDeleted - The node N that was deleted and, if E is not null, an
+    /// equivalent node E that replaced it.
+    virtual void NodeDeleted(SDNode *N, SDNode *E) = 0;
+
+    /// NodeUpdated - The node N that was updated.
     virtual void NodeUpdated(SDNode *N) = 0;
   };
   
index 8696bcf63b9a67632fc6d92bdb63c2f40ee3c137..0997840d852cc9c053fa8af2c3306239ca330238 100644 (file)
@@ -272,7 +272,7 @@ class VISIBILITY_HIDDEN WorkListRemover :
 public:
   explicit WorkListRemover(DAGCombiner &dc) : DC(dc) {}
   
-  virtual void NodeDeleted(SDNode *N) {
+  virtual void NodeDeleted(SDNode *N, SDNode *E) {
     DC.removeFromWorkList(N);
   }
   
index 61d0e45162245e5fb8e97ca7b70431d365b860dd..16c86936a6c0375adb99c700d37887bfdf06299a 100644 (file)
@@ -268,51 +268,6 @@ void DAGTypeLegalizer::AnalyzeNewNode(SDNode *&N) {
     Worklist.push_back(N);
 }
 
-void DAGTypeLegalizer::SanityCheck(SDNode *N) {
-  for (SmallVector<SDNode*, 128>::iterator I = Worklist.begin(),
-       E = Worklist.end(); I != E; ++I)
-    assert(*I != N);
-
-  for (DenseMap<SDOperand, SDOperand>::iterator I = ReplacedNodes.begin(),
-       E = ReplacedNodes.end(); I != E; ++I) {
-    assert(I->first.Val != N);
-    assert(I->second.Val != N);
-  }
-
-  for (DenseMap<SDOperand, SDOperand>::iterator I = PromotedNodes.begin(),
-       E = PromotedNodes.end(); I != E; ++I) {
-    assert(I->first.Val != N);
-    assert(I->second.Val != N);
-  }
-
-  for (DenseMap<SDOperand, SDOperand>::iterator
-       I = FloatToIntedNodes.begin(),
-       E = FloatToIntedNodes.end(); I != E; ++I) {
-    assert(I->first.Val != N);
-    assert(I->second.Val != N);
-  }
-
-  for (DenseMap<SDOperand, SDOperand>::iterator I = ScalarizedNodes.begin(),
-       E = ScalarizedNodes.end(); I != E; ++I) {
-    assert(I->first.Val != N);
-    assert(I->second.Val != N);
-  }
-
-  for (DenseMap<SDOperand, std::pair<SDOperand, SDOperand> >::iterator
-       I = ExpandedNodes.begin(), E = ExpandedNodes.end(); I != E; ++I) {
-    assert(I->first.Val != N);
-    assert(I->second.first.Val != N);
-    assert(I->second.second.Val != N);
-  }
-
-  for (DenseMap<SDOperand, std::pair<SDOperand, SDOperand> >::iterator
-       I = SplitNodes.begin(), E = SplitNodes.end(); I != E; ++I) {
-    assert(I->first.Val != N);
-    assert(I->second.first.Val != N);
-    assert(I->second.second.Val != N);
-  }
-}
-
 namespace {
   /// NodeUpdateListener - This class is a DAGUpdateListener that listens for
   /// updates to nodes and recomputes their ready state.
@@ -322,14 +277,15 @@ namespace {
   public:
     NodeUpdateListener(DAGTypeLegalizer &dtl) : DTL(dtl) {}
 
-    virtual void NodeDeleted(SDNode *N) {
-      // Ignore deletes.
+    virtual void NodeDeleted(SDNode *N, SDNode *E) {
       assert(N->getNodeId() != DAGTypeLegalizer::Processed &&
              N->getNodeId() != DAGTypeLegalizer::ReadyToProcess &&
              "RAUW deleted processed node!");
-#ifndef NDEBUG
-      DTL.SanityCheck(N);
-#endif
+      // It is possible, though rare, for the deleted node N to occur as a
+      // target in a map, so note the replacement N -> E in ReplacedNodes.
+      assert(E && "Node not replaced?");
+      for (unsigned i = 0, e = E->getNumValues(); i != e; ++i)
+        DTL.NoteReplacement(SDOperand(N, i), SDOperand(E, i));
     }
 
     virtual void NodeUpdated(SDNode *N) {
@@ -359,9 +315,9 @@ void DAGTypeLegalizer::ReplaceValueWith(SDOperand From, SDOperand To) {
   NodeUpdateListener NUL(*this);
   DAG.ReplaceAllUsesOfValueWith(From, To, &NUL);
 
-  // The old node may still be present in ExpandedNodes or PromotedNodes.
-  // Inform them about the replacement.
-  ReplacedNodes[From] = To;
+  // The old node may still be present in a map like ExpandedNodes or
+  // PromotedNodes.  Inform maps about the replacement.
+  NoteReplacement(From, To);
 }
 
 /// ReplaceNodeWith - Replace uses of the 'from' node's results with the 'to'
@@ -379,13 +335,13 @@ void DAGTypeLegalizer::ReplaceNodeWith(SDNode *From, SDNode *To) {
   // can potentially cause recursive merging.
   NodeUpdateListener NUL(*this);
   DAG.ReplaceAllUsesWith(From, To, &NUL);
-  
-  // The old node may still be present in ExpandedNodes or PromotedNodes.
-  // Inform them about the replacement.
+
+  // The old node may still be present in a map like ExpandedNodes or
+  // PromotedNodes.  Inform maps about the replacement.
   for (unsigned i = 0, e = From->getNumValues(); i != e; ++i) {
     assert(From->getValueType(i) == To->getValueType(i) &&
            "Node results don't match");
-    ReplacedNodes[SDOperand(From, i)] = SDOperand(To, i);
+    NoteReplacement(SDOperand(From, i), SDOperand(To, i));
   }
 }
 
@@ -402,7 +358,72 @@ void DAGTypeLegalizer::RemapNode(SDOperand &N) {
   }
 }
 
+/// ExpungeNode - If this is a deleted value that was kept around to speed up
+/// remapping, remove it globally now.  The only map that can have a deleted
+/// node as a source is ReplacedNodes.  Other maps can have deleted nodes as
+/// targets, but since their looked-up values are always immediately remapped
+/// using RemapNode, resulting in a not-deleted node, this is harmless as long
+/// as ReplacedNodes/RemapNode always performs correct mappings.  The mapping
+/// will always be correct as long as ExpungeNode is called on the source when
+/// adding a new node to ReplacedNodes, and called on the target when adding
+/// a new node to any map.
+void DAGTypeLegalizer::ExpungeNode(SDOperand N) {
+  SDOperand Replacement = N;
+  RemapNode(Replacement);
+  if (Replacement != N) {
+    // Remove N from all maps - this is expensive but extremely rare.
+    ReplacedNodes.erase(N);
+
+    for (DenseMap<SDOperand, SDOperand>::iterator I = ReplacedNodes.begin(),
+         E = ReplacedNodes.end(); I != E; ++I) {
+      if (I->second == N)
+        I->second = Replacement;
+    }
+
+    for (DenseMap<SDOperand, SDOperand>::iterator I = PromotedNodes.begin(),
+         E = PromotedNodes.end(); I != E; ++I) {
+      assert(I->first != N);
+      if (I->second == N)
+        I->second = Replacement;
+    }
+
+    for (DenseMap<SDOperand, SDOperand>::iterator I = FloatToIntedNodes.begin(),
+         E = FloatToIntedNodes.end(); I != E; ++I) {
+      assert(I->first != N);
+      if (I->second == N)
+        I->second = Replacement;
+    }
+
+    for (DenseMap<SDOperand, SDOperand>::iterator I = ScalarizedNodes.begin(),
+         E = ScalarizedNodes.end(); I != E; ++I) {
+      assert(I->first != N);
+      if (I->second == N)
+        I->second = Replacement;
+    }
+
+    for (DenseMap<SDOperand, std::pair<SDOperand, SDOperand> >::iterator
+         I = ExpandedNodes.begin(), E = ExpandedNodes.end(); I != E; ++I) {
+      assert(I->first != N);
+      if (I->second.first == N)
+        I->second.first = Replacement;
+      if (I->second.second == N)
+        I->second.second = Replacement;
+    }
+
+    for (DenseMap<SDOperand, std::pair<SDOperand, SDOperand> >::iterator
+         I = SplitNodes.begin(), E = SplitNodes.end(); I != E; ++I) {
+      assert(I->first != N);
+      if (I->second.first == N)
+        I->second.first = Replacement;
+      if (I->second.second == N)
+        I->second.second = Replacement;
+    }
+  }
+}
+
+
 void DAGTypeLegalizer::SetPromotedOp(SDOperand Op, SDOperand Result) {
+  ExpungeNode(Result);
   AnalyzeNewNode(Result.Val);
 
   SDOperand &OpEntry = PromotedNodes[Op];
@@ -411,6 +432,7 @@ void DAGTypeLegalizer::SetPromotedOp(SDOperand Op, SDOperand Result) {
 }
 
 void DAGTypeLegalizer::SetIntegerOp(SDOperand Op, SDOperand Result) {
+  ExpungeNode(Result);
   AnalyzeNewNode(Result.Val);
 
   SDOperand &OpEntry = FloatToIntedNodes[Op];
@@ -419,6 +441,7 @@ void DAGTypeLegalizer::SetIntegerOp(SDOperand Op, SDOperand Result) {
 }
 
 void DAGTypeLegalizer::SetScalarizedOp(SDOperand Op, SDOperand Result) {
+  ExpungeNode(Result);
   AnalyzeNewNode(Result.Val);
 
   SDOperand &OpEntry = ScalarizedNodes[Op];
@@ -437,6 +460,9 @@ void DAGTypeLegalizer::GetExpandedOp(SDOperand Op, SDOperand &Lo,
 }
 
 void DAGTypeLegalizer::SetExpandedOp(SDOperand Op, SDOperand Lo, SDOperand Hi) {
+  ExpungeNode(Lo);
+  ExpungeNode(Hi);
+
   // Lo/Hi may have been newly allocated, if so, add nodeid's as relevant.
   AnalyzeNewNode(Lo.Val);
   AnalyzeNewNode(Hi.Val);
@@ -458,6 +484,9 @@ void DAGTypeLegalizer::GetSplitOp(SDOperand Op, SDOperand &Lo, SDOperand &Hi) {
 }
 
 void DAGTypeLegalizer::SetSplitOp(SDOperand Op, SDOperand Lo, SDOperand Hi) {
+  ExpungeNode(Lo);
+  ExpungeNode(Hi);
+
   // Lo/Hi may have been newly allocated, if so, add nodeid's as relevant.
   AnalyzeNewNode(Lo.Val);
   AnalyzeNewNode(Hi.Val);
index db9adc619e34a4851cc20107ac3e5b4f7c824606..09ccfe99381183dacb5f7c5509d5eaac40d90a4c 100644 (file)
@@ -153,6 +153,12 @@ public:
     AnalyzeNewNode(N);
   }
 
+  void NoteReplacement(SDOperand From, SDOperand To) {
+    ExpungeNode(From);
+    ExpungeNode(To);
+    ReplacedNodes[From] = To;
+  }
+
 private:
   void AnalyzeNewNode(SDNode *&N);
 
@@ -160,6 +166,7 @@ private:
   void ReplaceNodeWith(SDNode *From, SDNode *To);
 
   void RemapNode(SDOperand &N);
+  void ExpungeNode(SDOperand N);
 
   // Common routines.
   SDOperand BitConvertToInteger(SDOperand Op);
@@ -391,9 +398,6 @@ private:
   SDOperand SplitOp_RET(SDNode *N, unsigned OpNo);
   SDOperand SplitOp_STORE(StoreSDNode *N, unsigned OpNo);
   SDOperand SplitOp_VECTOR_SHUFFLE(SDNode *N, unsigned OpNo);
-
-public:
-  void SanityCheck(SDNode *N);
 };
 
 } // end namespace llvm.
index b3acc9a39ecb24f477a532f7a3a741650b2e37c4..4f577fc269a2e5b998702ed12ce7e2ea81028afe 100644 (file)
@@ -495,7 +495,7 @@ void SelectionDAG::RemoveDeadNode(SDNode *N, DAGUpdateListener *UpdateListener){
     DeadNodes.pop_back();
     
     if (UpdateListener)
-      UpdateListener->NodeDeleted(N);
+      UpdateListener->NodeDeleted(N, 0);
     
     // Take the node out of the appropriate CSE map.
     RemoveNodeFromCSEMaps(N);
@@ -3886,7 +3886,7 @@ void SelectionDAG::ReplaceAllUsesWith(SDOperand FromN, SDOperand To,
       ReplaceAllUsesWith(U, Existing, UpdateListener);
       // U is now dead.  Inform the listener if it exists and delete it.
       if (UpdateListener) 
-        UpdateListener->NodeDeleted(U);
+        UpdateListener->NodeDeleted(U, Existing);
       DeleteNodeNotInCSEMaps(U);
     } else {
       // If the node doesn't already exist, we updated it.  Inform a listener if
@@ -3933,7 +3933,7 @@ void SelectionDAG::ReplaceAllUsesWith(SDNode *From, SDNode *To,
       ReplaceAllUsesWith(U, Existing, UpdateListener);
       // U is now dead.  Inform the listener if it exists and delete it.
       if (UpdateListener) 
-        UpdateListener->NodeDeleted(U);
+        UpdateListener->NodeDeleted(U, Existing);
       DeleteNodeNotInCSEMaps(U);
     } else {
       // If the node doesn't already exist, we updated it.  Inform a listener if
@@ -3978,7 +3978,7 @@ void SelectionDAG::ReplaceAllUsesWith(SDNode *From,
       ReplaceAllUsesWith(U, Existing, UpdateListener);
       // U is now dead.  Inform the listener if it exists and delete it.
       if (UpdateListener) 
-        UpdateListener->NodeDeleted(U);
+        UpdateListener->NodeDeleted(U, Existing);
       DeleteNodeNotInCSEMaps(U);
     } else {
       // If the node doesn't already exist, we updated it.  Inform a listener if
@@ -4002,9 +4002,9 @@ namespace {
                               SelectionDAG::DAGUpdateListener *chain)
       : Set(set), Chain(chain) {}
  
-    virtual void NodeDeleted(SDNode *N) {
+    virtual void NodeDeleted(SDNode *N, SDNode *E) {
       Set.remove(N);
-      if (Chain) Chain->NodeDeleted(N);
+      if (Chain) Chain->NodeDeleted(N, E);
     }
     virtual void NodeUpdated(SDNode *N) {
       if (Chain) Chain->NodeUpdated(N);
@@ -4086,7 +4086,7 @@ void SelectionDAG::ReplaceAllUsesOfValueWith(SDOperand From, SDOperand To,
     ReplaceAllUsesWith(User, Existing, &CSUL);
     
     // User is now dead.  Notify a listener if present.
-    if (UpdateListener) UpdateListener->NodeDeleted(User);
+    if (UpdateListener) UpdateListener->NodeDeleted(User, Existing);
     DeleteNodeNotInCSEMaps(User);
   }
 }