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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D77621#1999757 <https://reviews.llvm.org/D77621#1999757>, @browneee wrote:

> I resubmitted the report_fatal_error checks again under D77601 <https://reviews.llvm.org/D77601>
>
> http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588c3280c6&to=a30e7ea88e75568feed020aedae73c52de888835&stat=max-rss
>  http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588c3280c6&to=a30e7ea88e75568feed020aedae73c52de888835&stat=instructions
>
> Imo impact from this part is insignificant.


Ah, OK - thanks for noting that!

@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?

> 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)

> 2. More function calls. They also appear fairly optimizable to me.
> 
>   I may not have good insight into the actual optimization behavior here.

*nod* Didn't seem especially interesting.

> 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?)

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621