[llvm] r259419 - SmallSet/SmallPtrSet: Refuse huge Small numbers

[Philip Reames via llvm-commits]

My suspicion is that 64 or 128 is probably a better sweet spot just 
because of the indirect value of avoiding heap fragmentation. However, I 
don't have any evidence and don't care enough right now to actually ask 
you to change it.  :)  You approach seems reasonable, so let's run with 
that for the moment.

On 02/01/2016 03:58 PM, Matthias Braun wrote:
> As this was impossible as compiletime (except that compiletime benefitted in the <1% range for replacing some Small==128 users). To get at 32 I looked at the instruction count reported by valgrind/callgrind which includes the instruction for the linear search and the instructions spend in libc malloc and that was the best cutoff point overall.
> I have no problem setting this higher as my measurements may very well be dependent on the malloc implementation and cache effects.
>
> - Matthias
>
>> On Feb 1, 2016, at 3:52 PM, Philip Reames via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Having a cutoff seems entirely reasonable, but why 32?  Linear search performs surprisingly well and avoiding heap allocation can be valuable.  How'd you come up with the threshold chosen?
>>
>> Philip
>>
>> On 02/01/2016 02:05 PM, Matthias Braun via llvm-commits wrote:
>>> Author: matze
>>> Date: Mon Feb  1 16:05:16 2016
>>> New Revision: 259419
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259419&view=rev
>>> Log:
>>> SmallSet/SmallPtrSet: Refuse huge Small numbers
>>>
>>> These sets do linear searching in small mode; It is not a good idea to
>>> use huge numbers as the small value here, save people from themselves by
>>> adding a static_assert.
>>>
>>> Differential Revision: http://reviews.llvm.org/D16706
>>>
>>> Modified:
>>>      llvm/trunk/include/llvm/ADT/SmallPtrSet.h
>>>      llvm/trunk/include/llvm/ADT/SmallSet.h
>>>      llvm/trunk/lib/Target/CppBackend/CPPBackend.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/SmallPtrSet.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallPtrSet.h?rev=259419&r1=259418&r2=259419&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/SmallPtrSet.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/SmallPtrSet.h Mon Feb  1 16:05:16 2016
>>> @@ -329,6 +329,11 @@ public:
>>>   /// SmallPtrSetImplBase for details of the algorithm.
>>>   template<class PtrType, unsigned SmallSize>
>>>   class SmallPtrSet : public SmallPtrSetImpl<PtrType> {
>>> +  // In small mode SmallPtrSet uses linear search for the elements, so it is
>>> +  // not a good idea to choose this value too high. You may consider using a
>>> +  // DenseSet<> instead if you expect many elements in the set.
>>> +  static_assert(SmallSize <= 32, "SmallSize should be small");
>>> +
>>>     typedef SmallPtrSetImpl<PtrType> BaseT;
>>>       // Make sure that SmallSize is a power of two, round up if not.
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/SmallSet.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallSet.h?rev=259419&r1=259418&r2=259419&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/SmallSet.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/SmallSet.h Mon Feb  1 16:05:16 2016
>>> @@ -38,6 +38,11 @@ class SmallSet {
>>>     typedef typename SmallVector<T, N>::const_iterator VIterator;
>>>     typedef typename SmallVector<T, N>::iterator mutable_iterator;
>>>   +  // In small mode SmallPtrSet uses linear search for the elements, so it is
>>> +  // not a good idea to choose this value too high. You may consider using a
>>> +  // DenseSet<> instead if you expect many elements in the set.
>>> +  static_assert(N <= 32, "N should be small");
>>> +
>>>   public:
>>>     typedef size_t size_type;
>>>     SmallSet() {}
>>>
>>> Modified: llvm/trunk/lib/Target/CppBackend/CPPBackend.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/CppBackend/CPPBackend.cpp?rev=259419&r1=259418&r2=259419&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/CppBackend/CPPBackend.cpp (original)
>>> +++ llvm/trunk/lib/Target/CppBackend/CPPBackend.cpp Mon Feb  1 16:05:16 2016
>>> @@ -1651,8 +1651,8 @@ void CppWriter::printFunctionUses(const
>>>       // Print type definitions for every type referenced by an instruction and
>>>     // make a note of any global values or constants that are referenced
>>> -  SmallPtrSet<GlobalValue*,64> gvs;
>>> -  SmallPtrSet<Constant*,64> consts;
>>> +  SmallPtrSet<GlobalValue*,32> gvs;
>>> +  SmallPtrSet<Constant*,32> consts;
>>>     for (Function::const_iterator BB = F->begin(), BE = F->end();
>>>          BB != BE; ++BB){
>>>       for (BasicBlock::const_iterator I = BB->begin(), E = BB->end();
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits