Fix SelectionDAG::ReplaceAllUsesWith to behave correctly when
authorDan Gohman <gohman@apple.com>
Mon, 19 Jan 2009 21:44:21 +0000 (21:44 +0000)
committerDan Gohman <gohman@apple.com>
Mon, 19 Jan 2009 21:44:21 +0000 (21:44 +0000)
uses are added to the From node while it is processing From's
use list, because of automatic local CSE. The fix is to avoid
visiting any new uses.

Fix a few places in the DAGCombiner that assumed that after
a RAUW call, the From node has no users and may be deleted.

This fixes PR3018.

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

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
test/CodeGen/X86/pr3018.ll [new file with mode: 0644]

index 67a419aac9084c95f35779994061bef3b8291912..0d90fc0748798f85b90ddbace6f599c43865cfa5 100644 (file)
@@ -522,12 +522,17 @@ SDValue DAGCombiner::CombineTo(SDNode *N, const SDValue *To, unsigned NumTo,
     }
   }
   
-  // Nodes can be reintroduced into the worklist.  Make sure we do not
-  // process a node that has been replaced.
-  removeFromWorkList(N);
+  // Finally, if the node is now dead, remove it from the graph.  The node
+  // may not be dead if the replacement process recursively simplified to
+  // something else needing this node.
+  if (N->use_empty()) {
+    // Nodes can be reintroduced into the worklist.  Make sure we do not
+    // process a node that has been replaced.
+    removeFromWorkList(N);
   
-  // Finally, since the node is now dead, remove it from the graph.
-  DAG.DeleteNode(N);
+    // Finally, since the node is now dead, remove it from the graph.
+    DAG.DeleteNode(N);
+  }
   return SDValue(N, 0);
 }
 
@@ -658,12 +663,17 @@ void DAGCombiner::Run(CombineLevel AtLevel) {
     for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i)
       AddToWorkList(N->getOperand(i).getNode());
       
-    // Nodes can be reintroduced into the worklist.  Make sure we do not
-    // process a node that has been replaced.
-    removeFromWorkList(N);
+    // Finally, if the node is now dead, remove it from the graph.  The node
+    // may not be dead if the replacement process recursively simplified to
+    // something else needing this node.
+    if (N->use_empty()) {
+      // Nodes can be reintroduced into the worklist.  Make sure we do not
+      // process a node that has been replaced.
+      removeFromWorkList(N);
     
-    // Finally, since the node is now dead, remove it from the graph.
-    DAG.DeleteNode(N);
+      // Finally, since the node is now dead, remove it from the graph.
+      DAG.DeleteNode(N);
+    }
   }
   
   // If the root changed (e.g. it was a dead load, update the root).
index b83d537e372231673d0e60e53cb34bf1f8733269..d8124e6d7c34216c992f7e1676a4fabf1667a6bb 100644 (file)
@@ -4388,9 +4388,16 @@ void SelectionDAG::ReplaceAllUsesWith(SDValue FromN, SDValue To,
          "Cannot replace with this method!");
   assert(From != To.getNode() && "Cannot replace uses of with self");
 
-  while (!From->use_empty()) {
-    SDNode::use_iterator UI = From->use_begin();
+  // Iterate over all the existing uses of From. This specifically avoids
+  // visiting any new uses of From that arrise while the replacement is
+  // happening, because any such uses would be the result of CSE: If an
+  // existing node looks like From after one of its operands is replaced
+  // by To, we don't want to replace of all its users with To too.
+  // See PR3018 for more info.
+  SDNode::use_iterator UI = From->use_begin(), UE = From->use_end();
+  while (UI != UE) {
     SDNode *U = *UI;
+    do ++UI; while (UI != UE && *UI == U);
 
     // This node is about to morph, remove its old self from the CSE maps.
     RemoveNodeFromCSEMaps(U);
@@ -4437,9 +4444,12 @@ void SelectionDAG::ReplaceAllUsesWith(SDNode *From, SDNode *To,
   if (From == To)
     return;
 
-  while (!From->use_empty()) {
-    SDNode::use_iterator UI = From->use_begin();
+  // Iterate over just the existing users of From. See the comments in
+  // the ReplaceAllUsesWith above.
+  SDNode::use_iterator UI = From->use_begin(), UE = From->use_end();
+  while (UI != UE) {
     SDNode *U = *UI;
+    do ++UI; while (UI != UE && *UI == U);
 
     // This node is about to morph, remove its old self from the CSE maps.
     RemoveNodeFromCSEMaps(U);
@@ -4480,9 +4490,12 @@ void SelectionDAG::ReplaceAllUsesWith(SDNode *From,
   if (From->getNumValues() == 1)  // Handle the simple case efficiently.
     return ReplaceAllUsesWith(SDValue(From, 0), To[0], UpdateListener);
 
-  while (!From->use_empty()) {
-    SDNode::use_iterator UI = From->use_begin();
+  // Iterate over just the existing users of From. See the comments in
+  // the ReplaceAllUsesWith above.
+  SDNode::use_iterator UI = From->use_begin(), UE = From->use_end();
+  while (UI != UE) {
     SDNode *U = *UI;
+    do ++UI; while (UI != UE && *UI == U);
 
     // This node is about to morph, remove its old self from the CSE maps.
     RemoveNodeFromCSEMaps(U);
diff --git a/test/CodeGen/X86/pr3018.ll b/test/CodeGen/X86/pr3018.ll
new file mode 100644 (file)
index 0000000..7d335ee
--- /dev/null
@@ -0,0 +1,8 @@
+; RUN: llvm-as < %s | llc -march=x86 | grep {orl       \$1}
+
+define i32 @test(i32 %A) nounwind {
+  %B = or i32 %A, 1
+  %C = or i32 %B, 1
+  %D = and i32 %C, 7057
+  ret i32 %D
+}