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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
