dblaikie added a comment.

In D77621#1970015 <https://reviews.llvm.org/D77621#1970015>, @dexonsmith wrote:

> In D77621#1968647 <https://reviews.llvm.org/D77621#1968647>, @browneee wrote:
>
> > Do we want to increase the complexity of SmallVector somewhat? or do we 
> > want to keep the limit and affirm SmallVector is for small things?
>
>
> I don't think we should limit `SmallVector` to small things.  Most 
> `std::string` implementations also have the small storage optimization, but 
> they're not limited to small things.  Note that even `SmallVector<T,0>` has a 
> number of conveniences for LLVM over `std::vector` (such as extra API, 
> ability to use `SmallVectorImpl` APIs, and no pessimizations from exception 
> handling).
>
> Personally, I'm fine with splitting SmallVectorBase into 
> `SmallVectorBase<uintptr_t>` and `SmallVectorBase<uint32_t>` (on 32-bit 
> architectures, there's actually no split there).  There aren't APIs that take 
> a `SmallVectorBase` so there's no downside there.  It doesn't seem too bad to 
> me to do something like:
>
>   template <class SizeT> SmallVectorBase {
>     typedef SizeT size_type;
>     // ...
>   };
>   template <class T>
>   using SmallVectorSizeType =
>       std::conditional<sizeof(T) < 4, uintptr_t, uint32_t>;
>   template <class T> SmallVectorImpl :
>       SmallVectorBase<SmallVectorSizeType<T>> { ... };
>
>
> If the complexity is too much, I would personally prefer to have my patch 
> reverted (option 2 above) over making SmallVector stop working with large 
> byte arrays.


Fair enough - that complexity seems reasonably acceptable to me if you reckon 
the memory size benefits are still worthwhile (did you measure them on any 
particular workloads? Do we have lots of fairly empty SmallVectors, etc?) if 
they don't apply to smaller types like this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to