[PATCH] D118877: [DagCombine] Increase depth by number of operands to avoid a pathological compile time.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

I'm far from an expert at this (I don't know that much about TokenFactors and the DAGCombiner worklist).
But one thing that caught my attention was the code that https://github.com/llvm/llvm-project/issues/52858 is talking about in visitTokenFactor:

  SDValue DAGCombiner::visitTokenFactor(SDNode *N) {
    ...
    // If the sole user is a token factor, we should make sure we have a
    // chance to merge them together. This prevents TF chains from inhibiting
    // optimizations.
    if (N->hasOneUse() && N->use_begin()->getOpcode() == ISD::TokenFactor)
      AddToWorklist(*(N->use_begin()));

I'm wondering why, or rather if it is good idea that, we always add the user to the worklist here. Regardless if the current TokenFactor is simplified or not.
Normally I think we visit nodes bottom-up (or rather in topological order, but well that has puzzled me before because it seems a bit random, see https://discourse.llvm.org/t/shouldnt-dag-combines-be-applied-in-topological-order/59458), so we have probably just visited the using TokenFactor. Visiting that node again seems a bit redundant unless we actually perform some optimizations on the current TokenFactor.

So maybe one shouldn't just add the user to the worklist directly here, instead postponing it to the ends of the function and make it conditional on if any changes were made.

Afaict that gives a speedup to the example in this ticket. When it comes to the code example from https://github.com/llvm/llvm-project/issues/52858, then I couldn't really see any significant differences when using `-time-passes` even if removing the AddToWorklist completely (so either that reproducer won't work for the commit/target I used when testing, or I might have messed up my testing somehow).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118877