[llvm-commits] [cfe-commits] [PATCH][Review Request] EarlyCSE stack overflow - bugzilla 11794

[Lenny Maiorani]

On Jan 18, 2012, at 5:23 PM, Jakub Staszak wrote:

> I think that std::vector (instead of stack) can save some compilation time here, because it is included anyway.
> 
> - Kuba
> 
> On Jan 18, 2012, at 10:57 PM, Lenny Maiorani wrote:
> 
>> 
>> On Jan 18, 2012, at 2:34 PM, Eli Friedman wrote:
>> 
>>> On Wed, Jan 18, 2012 at 1:25 PM, Lenny Maiorani <lenny at colorado.edu> wrote:
>>>> Hi,
>>>> 
>>>> I found a stack overflow in EarlyCSE caused by recursion when provided a very large function. More information in bugzilla - http://llvm.org/bugs/show_bug.cgi?id=11794
>>>> 
>>>> Attached is a patch. Please review and I will commit once it is approved.
>>>> 
>>>> The patch simply changes the algorithm from being a recursive pre-order depth-first traversal to the same traversal, but using a list as a stack of nodes to process.
>>> 
>>> The idea looks like it's in the right direction. We generally try to avoid
>>> std::list in LLVM; see
>>> http://llvm.org/docs/ProgrammersManual.html#ds_sequential .
>>> 
>>> In the future, please don't send patches that don't touch
>>> clang-specific code to cfe-commits.
>>> 
>>> -Eli
>> 
>> Hi Eli,
>> 
>> The reason I chose to use std::list here is that it supports splice. Perhaps a std::stack implementation is preferred here?
>> 
>> Attached is a patch implementing that...
>> 
>> -Lenny
>> 
>> <earlycse-stack.diff>

Hi Jakub, Eli, others...

As I determined earlier, there is a clear problem in the EarlyCSE optimizer. It dumps core because of a stack overflow caused by really long functions. Attached is a python script (clang-earlycse-generator.py) which generates a program which the TOT LLVM/Clang does not compile due to this problem.

Previously, I submitted a patches implementing fixes using std::list and std::stack. These were rejected in favor of not using std::list and using std::vector respectively. 

std::vector is included already in EarlyCSE.cpp so Jakub was suggesting that to increase compile time performance of that file by avoiding the need to include (parse, etc...) std::stack. However, std::vector is not going to perform as well in this scenario because it does more reallocs than std::stack as it grows.

Before I blindly stated that I thought std::stack or std::deque would be better, I decided I would verify this. I wrote a small test of a few different implementation (std::list, std::stack, std::deque, std::vector, and std::deque using iterator-insert) attempting to perform the same operations as the necessary ones in EarlyCSE.cpp. It is distilled down to just the performance of the actual data structures under the assumed conditions. Attached (stack-test.cpp).

As it turns out, the "best" implementation in terms of performance is std::deque using the "iterator-insert", by a wide margin. On my lowly laptop, after compiling the test with 'clang++ -O2', I get the following results:
List: 92.473490 seconds
Stack: 15.575143 seconds
Deque: 9.129154 seconds
Deque iter insert: 7.384741 seconds
Vector: 13.387473 seconds

After this analysis, I recommend the following attached (earlycse-deque-iter.diff) patch. I will also add this analysis and patch bz11794. 

It is my belief that trading off compile time performance of LLVM/Clang for runtime performance is a clear win. Besides, after the first compile, subsequent self-hosted compilations should get better performance over the entire build than is lost by in the include of std::deque. The code is also simpler using this method because no copy is done to reverse the structure created by iterating forward.

Provided there are no objections this time around, I will commit.
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-earlycse-generator.py
Type: text/x-python-script
Size: 506 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120119/7d6e8a26/attachment.bin>
-------------- next part --------------
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stack-test.cpp
Type: application/octet-stream
Size: 6048 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120119/7d6e8a26/attachment.obj>
-------------- next part --------------
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: earlycse-deque.diff
Type: application/octet-stream
Size: 2245 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120119/7d6e8a26/attachment-0001.obj>
-------------- next part --------------