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

[Chris Lattner]

On Nov 24, 2010, at 1:21 PM, Cameron Zwarich wrote:

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

Jakob's right, SmallPtrSet was really intended to be a stack object.  Typically if you overflow a stack object, it is because you're in the "large" case, and you might as well go for one big allocation to avoid the reallocation.

However, I guess it does make sense for "small" objects to be used in (already expensive) node-based containers like std::map.  I wouldn't be opposed to adding a template argument to control this, and have it get passed down as a bool to grow().

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

Another concern: does anything depend on iteration over DF sets being in pointer order?  SmallPtrSet doesn't provide stable iteration.

There are other low-hanging opportunities to speed up the existing dom frontiers implementation: the std::set in compareDomSet can trivially be converted to SmallPtrSet (and ideally moved out of line :) for example.

In any case, your patch looks good to me Cameron, please apply!  Please do look into the grow() issue though.

-Chris