[llvm-commits] Review request: dominance frontier computation speedup

[Cameron Zwarich]

On Nov 24, 2010, at 3:21 PM, Jakob Stoklund Olesen wrote:

> This is actually in the non-templated SmallPtrSetImpl::Grow function.
> 
> I don't know which method was used to arrive at the number 128. The Small* classes are actually not designed to be compact despite the name. They are designed to work well as stack objects, avoiding malloc for typical loads.
> 
> Presumably, the large initial size was chosen to avoid multiple reallocations and re-hashings as the set grows. When a SmallPtrSet is used as a one-off stack object, that is the right thing to do.

We could template it to add this parameter, so the common case can be shared, but that's probably not a great idea. 

> A further concern is that the dominance frontier algorithm erases elements from the sets, so they are likely to be over-allocated anyway.

It's only DominanceFrontier::splitBlock() that erases elements from sets. The actual DF computation doesn't erase any elements.

> Perhaps there exists a third data structure that would be both compact and fast? Is it necessary to be able to enumerate a node's dominance frontier? Is it enough to be able to answer "Is A in the dominance frontier of B?"

Both the IDF algorithm and RegionInfo need to iterate over the elements of DFs. It would be nice to use SmallSet instead, though.

>> I also noticed that there is an extra pointer wasted for SmallStorage:
>> 
>> /// SmallStorage - Fixed size storage used in 'small mode'.  The extra element
>> /// ensures that the end iterator actually points to valid memory.
>> const void *SmallStorage[SmallSizePowTwo+1];
>> 
>> Isn't a pointer to 1 past valid memory valid for comparisons in C++? Nothing should be dereferencing it.
> 
> Author: Duncan Sands <baldrick at free.fr>
> Date:   Tue Jun 29 13:12:02 2010
> 
>    Fix a buffer overflow noticed by gcc-4.6: zero is written into
>    SmallArray[SmallSize] in the SmallPtrSetIteratorImpl, and this is
>    one off the end of the array.  For those who care, right now gcc
>    warns about writing off the end because it is confused about the
>    declaration of SmallArray as having length 1 in the parent class
>    SmallPtrSetIteratorImpl.  However if you tweak code to unconfuse
>    it, then it still warns about writing off the end of the array,
>    because of this buffer overflow.  In short, even with this fix
>    gcc-4.6 will warn about writing off the end of the array, but now
>    that is only because it is confused.

I see. I guess it's probably not worth eliminating that.

Cameron