[PATCH] Add SmallDenseSet

Chandler Carruth chandlerc at google.com
Wed Mar 18 17:53:55 PDT 2015


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? 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/2897846c/attachment.html>


More information about the llvm-commits mailing list