[PATCH] Add SmallDenseSet

[Daniel Berlin]

On Wed, Mar 18, 2015 at 5:53 PM, Chandler Carruth <chandlerc at google.com>
wrote:

> Meta comment, use Phabricator?
>
> On Wed, Mar 18, 2015 at 5:36 PM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
>
>> This just makes SmallDenseSet a thing, with no actual implementation
>> changes, just template parameter changes.
>>
>> My C++ fu is weak on the "how do i make this happen front", so feel free
>> to point out a better way to do this.
>>
>
> The change is fine in and of itself. We should add a unittest so that we
> actually make sure this code works and is compiling on every compiler.
>
> However...
>
>
>
>>
>> SmallDenseSet is being used in a soon-to-be submitted out of tree patch.
>>
>
> I'm sad you're using DenseSet at all. Does SmallPtrSet not work?
>
It was for a std::pair.
SmallSet was significantly slower (IE 5% of the time for this pass) for
almost any number i tried.

Actually, I think i can rearchitect this to not use any set at all for the
moment :)
So i'm going to bail on this patch until it becomes necessary, since it
seems simple enough to readd later.



> The implementation of DenseSet is... pretty inefficient. We should really
> build a variant of this which is its own thing rather than being layered on
> top of DenseMap, and potentially layer DenseMap on top of the set as is
> usually done. I've not really rushed off to do this because I almost never
> find that DenseSet is really what I need. I almost always actually need a
> SmallPtrSet or a sorted and uniqued vector. Anyways, that doesn't mean we
> shouldn't make the existing code for SmallDenseSet work or delete it, and
> check in a unittest that covers it if the code is going to stay.
>
> -Chandler
>
>
>
>>
>> (changed parts were reformatted with clang-format)
>> ---
>>  include/llvm/ADT/DenseSet.h | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/llvm/ADT/DenseSet.h b/include/llvm/ADT/DenseSet.h
>> index d340240..aee825c8 100644
>> --- a/include/llvm/ADT/DenseSet.h
>> +++ b/include/llvm/ADT/DenseSet.h
>> @@ -35,10 +35,12 @@ public:
>>  }
>>
>>  /// DenseSet - This implements a dense probed hash-table based set.
>> -template<typename ValueT, typename ValueInfoT = DenseMapInfo<ValueT> >
>> +template <typename ValueT, typename ValueInfoT = DenseMapInfo<ValueT>,
>> +          typename DenseMapT =
>> +              DenseMap<ValueT, detail::DenseSetEmpty, ValueInfoT,
>> +                       detail::DenseSetPair<ValueT>>>
>>  class DenseSet {
>> -  typedef DenseMap<ValueT, detail::DenseSetEmpty, ValueInfoT,
>> -                   detail::DenseSetPair<ValueT>> MapTy;
>> +  typedef DenseMapT MapTy;
>>    static_assert(sizeof(typename MapTy::value_type) == sizeof(ValueT),
>>                  "DenseMap buckets unexpectedly large!");
>>    MapTy TheMap;
>> @@ -157,6 +159,14 @@ public:
>>    }
>>  };
>>
>> +/// SmallDenseSet - This is the SmallDenseMap version of DenseSet
>> +template <typename ValueT, typename ValueInfoT = DenseMapInfo<ValueT>,
>> +          unsigned InlineBuckets = 4>
>> +using SmallDenseSet =
>> +    DenseSet<ValueT, ValueInfoT,
>> +             SmallDenseMap<ValueT, detail::DenseSetEmpty, InlineBuckets,
>> +                           ValueInfoT, detail::DenseSetPair<ValueT>>>;
>> +
>>  } // end namespace llvm
>>
>>  #endif
>> --
>> 2.3.3
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150318/9b19887f/attachment.html>