[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[David Blaikie via Phabricator via llvm-commits]

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