[PATCH] [SDAG] Make the DAGCombine worklist not grow endlessly due to duplicate insertions.

[Tim Northover]

Hi Chandler,

I've taken a look at the ARM changes, and I think they're mostly innocuous.

On the patch as a whole, I don't think relying on nodes being visited in a particular order for good codegen is a good idea anyway. It usually means you're only handling one particular edge-case of a more general construct. So I wouldn't worry too much about that myself. Still annoying for the person like you who comes along to try & make things better though.

I can't think of any problems with the new implementation, though I'm probably not the best person around for picking the right data structure. Hopefully someone else is better there.

Cheers.

Tim.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:101-107
@@ -99,8 +100,9 @@
     //
     // The set contains the items on the worklist, but does not
     // maintain the order they should be visited.
     //
     // The vector maintains the order nodes should be visited, but may
     // contain duplicate or removed nodes. When choosing a node to
     // visit, we pop off the order stack until we find an item that is
     // also in the contents set. All operations are O(log N).
+    SmallVector<SDNode*, 64> Worklist;
----------------
This comment now seems out of date.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1096
@@ +1095,3 @@
+      for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i)
+        if (N != N->getOperand(i).getNode())
+          Nodes.insert(N->getOperand(i).getNode());
----------------
Is this possible? Isn't that an immediate cycle?

================
Comment at: test/CodeGen/ARM/sxt_rot.ll:12-13
@@ -11,3 +11,4 @@
 ; CHECK: test1
-; CHECK: sxtb r0, r0, ror #8
+; CHECK: lsr r0, r0, #8
+; CHECK: sxtb r0, r0
   %B = lshr i32 %A, 8
----------------
This doesn't look ideal, but it's just a deficiency in the ARM patterns. I've got a fix ready to go, but I'll wait until this is in to avoid giving you conflicts to resolve or anything.

http://reviews.llvm.org/D4616