[llvm-commits] [llvm] r62533 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/X86/pr3018.ll

[Dan Gohman]

Author: djg
Date: Mon Jan 19 15:44:21 2009
New Revision: 62533

URL: http://llvm.org/viewvc/llvm-project?rev=62533&view=rev
Log:
Fix SelectionDAG::ReplaceAllUsesWith to behave correctly when
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.

Added:
    llvm/trunk/test/CodeGen/X86/pr3018.ll
Modified:
    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
    llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=62533&r1=62532&r2=62533&view=diff

==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Mon Jan 19 15:44:21 2009
@@ -522,12 +522,17 @@
     }
   }
   
-  // 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 @@
     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).

Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp?rev=62533&r1=62532&r2=62533&view=diff

==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Mon Jan 19 15:44:21 2009
@@ -4388,9 +4388,16 @@
          "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 @@
   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 @@
   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);

Added: llvm/trunk/test/CodeGen/X86/pr3018.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/pr3018.ll?rev=62533&view=auto

==============================================================================
--- llvm/trunk/test/CodeGen/X86/pr3018.ll (added)
+++ llvm/trunk/test/CodeGen/X86/pr3018.ll Mon Jan 19 15:44:21 2009
@@ -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
+}