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

[James Molloy]

Hi Chris,

 

Thanks for the quick review! J

 

1.       I've used SmallPtrSet with a size of 64 as a wild finger-in-the-air
guess. Do you reckon that's about right?

 

2.       Fixed, thanks.

 

3.       Omission on my part - removed during my changes and forgot to
re-add it when I found it was actually rather important L

 

4.       I've modified the comments. They still compare the algorithm versus
a naive/obvious approach because I feel that I have to justify the extra
complexity. Are they OK now?

 

 

Cheers,

 

James

 

From: Chris Lattner [mailto:clattner at apple.com] 
Sent: 15 February 2012 11:50
To: James Molloy
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [PATCH] Reduce complexity of DAGCombiner

 

 

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/940b4a90/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: addtoworklist.2.patch
Type: application/octet-stream
Size: 3834 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120215/940b4a90/attachment.obj>