[PATCH] D93779: ADT: Fix reference invalidation in SmallVector::push_back and single-element insert

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

@xbolva00, thanks for reporting the regression. After the performance analysis was finished in https://reviews.llvm.org/D91837, I split the patch up for more detailed review, and in responding to review comments I somehow lost the critical check against `TakesParamByValue`.

Also, @nikic, thanks for the reverts yesterday when I missed the updates here.

https://reviews.llvm.org/D94800 adds back the critical check.

In D93779#2498509 <https://reviews.llvm.org/D93779#2498509>, @dblaikie wrote:

> In D93779#2498501 <https://reviews.llvm.org/D93779#2498501>, @lebedev.ri wrote:
>
>> Why do we even want to bend over backwards and try to handle broken code correctly,
>> at the cost of regressed performance for everything,
>> instead of just adding defensive asserts against such problems,
>> and letting/telling the user fix the code?
>
> I believe the standard library supports this, so I think it's a somewhat awkward limitation to have a standard-library-like container that misses that. (of course the small-mode isn't entirely compatible with std::vector, etc anyway - so some divergence is possible)

Exactly, this is part of rectifying some unnecessary awkwardness in `SmallVector`. I'm 100% agreed it's not worth it with the compile-time impact you reported, but this approach seems to get the benefits without significant cost (when I don't drop critical checks...). You can see the numbers in https://reviews.llvm.org/D91837.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:232
+    int64_t Index = -1;
+    if (LLVM_UNLIKELY(This->isReferenceToStorage(&Elt))) {
+      ReferencesStorage = true;
----------------
The cause of the regression was dropping the check against `TakesParamByValue` on this line when I moved this function to `SmallVectorTemplateCommon`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93779