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

[Jakob Stoklund Olesen]

On Nov 24, 2010, at 11:37 AM, Cameron Zwarich wrote:

> On Nov 24, 2010, at 12:44 PM, Jakob Stoklund Olesen wrote:
> 
>> When a SmallPtrSet overflows its 'small' size, it allocates space for 128 pointers. That uses more memory than a std::set for up to 32 entries which is probably most cases.
> 
> I wouldn't have guessed that the jump was so drastic. Could it be controlled with a template parameter? I can run some stats to guess a good value.

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.

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

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?"

> 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.
    
    
    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@107200 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/ADT/SmallPtrSet.h b/include/llvm/ADT/SmallPtrSet.h
index 48637f3..f1405fa 100644
--- a/include/llvm/ADT/SmallPtrSet.h
+++ b/include/llvm/ADT/SmallPtrSet.h
@@ -233,7 +233,7 @@ template<class PtrType, unsigned SmallSize>
 class SmallPtrSet : public SmallPtrSetImpl {
   // Make sure that SmallSize is a power of two, round up if not.
   enum { SmallSizePowTwo = NextPowerOfTwo<SmallSize>::Val };
-  void *SmallArray[SmallSizePowTwo];
+  void *SmallArray[SmallSizePowTwo+1];
   typedef PointerLikeTypeTraits<PtrType> PtrTraits;
 public:
   SmallPtrSet() : SmallPtrSetImpl(SmallSizePowTwo) {}


> 
>> Actually, I think Chris wants to get rid of dominance frontiers entirely because of the memory requirements.
> 
> Yeah, Chris mentioned this and I have started looking at some other phi placement algorithms, at least for cases other than the first SRoA / mem2reg. I just want a fair comparison so I am speeding up dominance frontiers first.
> 
> Cameron

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1929 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101124/7a739d4e/attachment.bin>