[PATCH] D33587: [DAGCombine] Do several rounds of combine.

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix marked an inline comment as done.
deadalnix added a comment.

After thinking more about this, I do not think going bottom up is a good idea. All patterns match a node + its operands, and so benefit from operand to be combined themselves already. I do not think changing all the patterns to match use rather than operands is a good idea. This is a ton of work, and this is unclear there is any benefit at all.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1437
+      // this won't repeatedly process the same operand.
+      CombinedNodes.insert(N);
+      for (const SDValue &ChildN : N->op_values())
----------------
deadalnix wrote:
> craig.topper wrote:
> > Don't you need to reset CombineNodes on each iteration?
> Both would be correct but semantic obviously differ. Let me investigate this. Good catch.
So I was playing with reseting/not reseting this and even removing it altogether.

The first thing worth noticing is that the intent here is very similar, but more limited in scope. However, result sometime differs depending on if this is executed or not. It is due to various patterns in there depends on execution order - something I've noticed before, for instance in D41235 .

I do not think we need to reset it as all existing nodes are inserted at the start in the worklist, so this ends up only adding nodes to the worklist that have been created by a precedent successful combine. I notice zero codegen difference when reseting the set between iteration, so it seems to just create more work without any benefit.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D33587/new/

https://reviews.llvm.org/D33587