[llvm-commits] [PATCH] Reduce complexity of DAGCombiner

[Chris Lattner]

On Feb 15, 2012, at 2:57 AM, James Molloy wrote:

> Hi,
>  
> One of PlumHall’s testcases generates a function with half a million SDNodes. This brings out the O(n) complexity in DAGCombiner::AddToWorkList() and means that in debug mode the test has not been seen to complete compilation, and in release mode it takes several hours. KCacheGrind showed 99% of the time is being spent in std::remove, doing linear scans of the DAGCombiner’s worklist during AddToWorkList.

Wow, that's bad :), I'm surprised that this terrible algorithm lasted so long.

> My solution was to allow duplicate items in the vector, and on every pop to query the set to see if the item should actually be in the vector. If not, discard and grab the next item. This means that the vector could potentially grow larger than it would previously.

The approach makes sense.  Some requests on the patch:

1. Please use SmallPtrSet<> instead of std::set, it will be much more efficient (and may help your timings even more).  See also: http://llvm.org/docs/ProgrammersManual.html#ds_set

2. In removeFromWorkList, you can just call WorkListContents.erase(N) instead of find+erase.

3. You removed the comment from AddToWorkList about making sure that the thing added is next-to-be-processed.  As you discovered, this is important, please keep the comment.

4. The comments you added are great, but should be written explaining the algorithm your adding, not explaining a delta from the old code.  When this goes in the new algorithm will be truth, and the comments should explain why it works this way.

Thanks for finding and fixing this James!

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120215/5e470c62/attachment.html>