[PATCH] D127115: [RFC][DAGCombine] Make sure combined nodes are added back to the worklist in topological order.

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added a comment.

> But yes, I think this patch would help in situations like you describe. Can you apply the patch to confirm? (Without the test diffs, this is just a very small patch to DAGCombiner.)

Unfortunately, the target I'm observing the issue on is out-of-tree. Something similar can be found in ARMTargetLowering::PerformBRCONDCombine:

  // (brcond Chain BB ne CPSR (cmpz (and (cmov 0 1 CC CPSR Cmp) 1) 0))
  // -> (brcond Chain BB CC CPSR Cmp)
  if (CC == ARMCC::NE && LHS.getOpcode() == ISD::AND && LHS->hasOneUse() &&
      LHS->getOperand(0)->getOpcode() == ARMISD::CMOV &&
      LHS->getOperand(0)->hasOneUse()) {

Here, the `and` is redundant and can be optimized out. If this happens before `ARMISD::BRCOND` is visited, the pattern won't match. I tried to add another pattern without the `and`, but no luck -- all tests pass. Looks like the branch is always visited first here.

There is also `IsCMPZCSINC` which seems to deal with unpredictable visit order:

  // Ignore any `And 1` nodes that may not yet have been removed. We are
  // looking for a value that produces 1/0, so these have no effect on the
  // code.
  while (CSInc.getOpcode() == ISD::AND &&
         isa<ConstantSDNode>(CSInc.getOperand(1)) &&
         CSInc.getConstantOperandVal(1) == 1 && CSInc->hasOneUse())
    CSInc = CSInc.getOperand(0);

If the DAG was traversed in post-order, this loop won't be necessary. The loop was added in D115176 <https://reviews.llvm.org/D115176>. Quote:

> Those extra And 1's will be removed later, because we know they don't do anything. By that point we may not re-visit this node to do this transform, though. Hence the improvement in some of the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127115