[PATCH] [SDAG] Make the DAGCombine worklist not grow endlessly due to duplicate insertions.
t.p.northover at gmail.com
Tue Jul 22 06:42:48 PDT 2014
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.
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())
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.
More information about the llvm-commits