[PATCH] D53289: [DAGCombiner] Limit the number of chains in findBetterChains().

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.

Oh good catch. This is O(N^3) for long chains because we try every prefix of the chain, and because the chain is so long our chain improvements gives up and noops, so we just redo the same computation over and over. Cutting the length to any reasonable size makes it O(N^2). I suspect your observed merge size requirement is to deal with our incomplete handling of chain SubDAG in store merge (just a children of a single TF potentially skipping some loads), because it shouldn't matter otherwise (See (1)).

Although, maybe what we should actually do is just run up the chain and mark make sure each store is to a disjoint part of memory (which should be just a range check as they all had related pointers) and then just directly rewrite them into as fully parallel structure chained to a TokenFactor. That should be O(N) with a much smaller constant than this.

(1) I wrote a cleanup patch https://reviews.llvm.org/D31068 some time ago that never got around to chasing down the reviewers to care. It lets us merge stores from a chain of stores where we gave up on better aliasing. That should remove any burrs from giving up merges too soon. I've added you as a reviewer.



================
Comment at: include/llvm/CodeGen/TargetLowering.h:2598
+  // non-consecutive stores to be merged.
+  unsigned FindBetterChainsMaxChains;
+
----------------
I wrote some a cleanup patch some time ago that never got around to chasing down the reviewers to care. It incrementally allows us to deal with chains of stores in which should remove any burrs from giving up merges too soon, so we shouldn't need to have a target-dependenct depth check.




Repository:
  rL LLVM

https://reviews.llvm.org/D53289