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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D77621#1999957 <https://reviews.llvm.org/D77621#1999957>, @dblaikie wrote:

> @nikic any sense of the noise floor/level on these measurements? It doesn't /look/ like there's much left in this that would cause problems. & I assume these measurements were made on an optimized build (so we don't have to try to improve the unoptimized code?


The measurements are on an optimized build (default LLVM release build, so no LTO). The noise level on the "instructions" metric is very low, so that changes above 0.1% tend to be significant. The compile-time regression on the original change definitely wasn't noise (but the change from D77601 <https://reviews.llvm.org/D77601> is in the noise).

>> Other pieces I see as possibly impacting compile time are:
>> 
>> 1. This correction to SmallVectorTemplateCommon::max_size().   But SizeTypeMax() is static constexpr, this seems like it could still be optimized to a constant.
>> 
>>   ```
>> 2. size_type max_size() const { return size_type(-1) / sizeof(T); } +  size_type max_size() const { +    return std::min(this->SizeTypeMax(), size_type(-1) / sizeof(T)); +  } ```
> 
> Perhaps you could move the value computation into a constexpr variable & just return that as needed. (could be a static local constexpr, I guess - to avoid the issues around linkage of constexpr member variables)

The use of a function rather than a static constexpr for SizeTypeMax() was my first thought as well. It seems pretty weird to me, but maybe it's enough to fall one the wrong side of some inlining heuristic.

The only other thing that comes to mind is that grow_pod() moved into the header, which might have negative effects. It should be possible to avoid that by providing explicit template instantiations for uint32_t and uintptr_t in the cpp file.

I'll try to figure out what the cause is, but might take me a few days.

>> I don't think the automatic switch of the size/capacity field has been justified well. We have plenty of SmallVectors in LLVM that are, indeed, small. There is no way an MCRelaxableFragment will ever end up storing a single instruction that is 4G large.
> 
> @nikic - can you explain the relevance of this ^ (as @dexonsmith pointed out, MCRelaxableFragment doesn't look like it would be affected by this change - is there something we're missing about that?)

MCRelaxableFragment also contains a SmallVector<char>. I used this as an example where we use a SmallVector<char> with a very low upper bound on the size. (This example is not great, because the structure is already large for other reasons.)

>> Similarly, I'm not really convinced about handling this in SmallVector at all. The original change here just used an std::vector in the one place where this has become an issue. That seems like a pretty good solution until there is evidence that this is really a more widespread problem.
> 
> I'm inclined to go with @dexonsmith's perspective here, as the author of the original change & the general attitude that SmallVector should support this kind of use case.

Okay, I'm basically fine with that, if it is our stance that SmallVector should always be preferred over std::vector. Not really related to this revision, but it would probably help to do some renaming/aliasing to facilitate that view. Right now, the number of `SmallVector<T, 0>` uses in LLVM is really small compared to the `std::vector<T>` uses (100 vs 6000 based on a not very accurate grep). I think part of that is in the name, and calling it `using Vector<T> = SmallVector<T, 0>` and `using VectorImpl<T> = SmallVectorImpl<T>` would make it a lot more obvious that this is our preferred general purpose vector type, even if the stored data is not small.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621