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

Clement Courbet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 16 08:25:26 PDT 2018


courbet added a comment.

In https://reviews.llvm.org/D53289#1265550, @niravd wrote:

> 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)).


I'm not sure I understand what (1) would change to the situation ?

> 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.

Note that this patch fixes a pathological case which is quite theoretical; the limit is never hit on any of the test-suite basic blocks.
Actually with this change it takes a basic block of more than ten thousand stores to reach the current compile time with hundreds of blocks, even with `-combiner-global-alias-analysis`.

> (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.




Repository:
  rL LLVM

https://reviews.llvm.org/D53289





More information about the llvm-commits mailing list