[PATCH] D91467: ADT: Take small enough, trivially copyable T by value in SmallVector (otherwise, assert)

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

In D91467#2401023 <https://reviews.llvm.org/D91467#2401023>, @dexonsmith wrote:

> In D91467#2400888 <https://reviews.llvm.org/D91467#2400888>, @dexonsmith wrote:
>
>> Note that the new assertions caught a bug in llvm/utils/TableGen/CodeGenSchedule.cpp and I found another related one by chance (not caught by these assertions though, since `emplace_back` is opaque). I'm still building, so once the tests run I assume there will be a number of other failures.
>
> Actually, that was it. Probably helps that for small `T` the invalidation was "fixed". Maybe there would be more failures on 32-bit platforms, and/or on a bootstrap; I'll run a bootstrap before committing but don't want to spend the time if this is DOA anyway.
>
> In D91467#2395340 <https://reviews.llvm.org/D91467#2395340>, @silvas wrote:
>
>> Do you have any performance and binary size results?
>
> Nope, happy to collect something though. What do you think we need to see?
>
> Note that right now the "fix invalidation" and "assert if invalid" are tied together in the same patch. Of course they're separable, but I'd be tempted to keep them together if there's a path to landing both. But if the optimization is DOA, I imagine we'd still want the assertion (and fixes for any extra assertion failures).

I think we can probably get a good signal from:

1. Clang execution time compiling some cpp file at by-value thresholds of `0`, `1 * sizeof(void*)`, `2 * sizeof(void*)` would be enough to get a signal on whether this is measurable performance wise.
2. Clang binary size for those 3 cases.


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

https://reviews.llvm.org/D91467