[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Apr 18 03:46:01 PDT 2020


nikic reopened this revision.
nikic added a comment.
This revision is now accepted and ready to land.

I have reverted this change, because it causes a 1% compile-time <http://llvm-compile-time-tracker.com/compare.php?from=73b7dd1fb3c17a4ac4b1f1e603f26fa708009649&to=b8d08e961df1d229872c785ebdbc8367432e9752&stat=instructions> and memory usage <http://llvm-compile-time-tracker.com/compare.php?from=73b7dd1fb3c17a4ac4b1f1e603f26fa708009649&to=b8d08e961df1d229872c785ebdbc8367432e9752&stat=max-rss> regression. The memory usage regression is probably fine given what this change does, but the compile-time regression is not. (For context, this pretty much undoes the wins that the recent removal of waymarking gave us.)

Some notes:

- Can you please split out the fix to grow() into a separate revision? It does not seem related to the main change, and reduces surface area.
- 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.
- 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.

But in any case, my primary concern here is the compile-time regression, and it's not immediately clear which part of the change it comes from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621





More information about the llvm-commits mailing list