[PATCH] D115443: [ADT] Weaken the initialization condition in SmallVector::resize_for_overwrite

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D115443#3253101 <https://reviews.llvm.org/D115443#3253101>, @jplayer-nv wrote:

> In D115443#3252648 <https://reviews.llvm.org/D115443#3252648>, @dblaikie wrote:
>
>> It can be/probably is useful for the API to be there, but be a no-op - so that it can be used in generic code (eg: in templates that don't know if they're operating on trivial or non-trivial element types, they call it, then carry on - for non-trivial types it doesn't do anything, for trivial ones it provides some performance improvement opportunities).
>
> I generally agree with everything you're saying.  Just wanted to point out that calling `resize_for_overwrite()` in template code can be worked around with a helper template that has a POD specialization:
>
> https://godbolt.org/z/5s5srhx4b
>
> I do agree that keeping `resize_for_overwrite()` available for non-pod types simplifies templated callers by avoiding this extra layer, but the desired effect is still possible when `resize_for_overwrite()` is only callable on PODs.

Yep - though once we'd done that we'd end up basically back where we were before in terms of the design space (though with a more explicit indication of which code is intentionally trying to be generic over POD and non-POD and which code is not, which has some value - but my tendency is that it doesn't feel like it carries enough benefit to be worth the added complexity)

> Another argument to disable for non-POD is code evolution.  Suppose you write proper code around a `SmallVector` of a POD type (call it `SmallVector<Foo>`).  You use `resize_for_overwrite()` like you're supposed to.  Later someone adds a constructor to `Foo`... suddenly your nice efficient code is now less efficient because of a very natural code evolution.
>
> Leaving `resize_for_overwrite()` as an alias to `resize()` in that case silently continues to function with unnecessary overhead (assuming there's a follow-on init loop).  If this evolution becomes a compile failure, then the writer of the `Foo` constructor must deal with all the old `resize_for_overwrite()` callsites (hopefully code them as efficiently as possible).

Yeah, if it wasn't a generic caller like above this would be an extra reminder to revisit this code, and maybe change it to "resize" if you realize the non-triviality is more important than the resize_for_overwrite (or to find another way rather than making it trivial). You could get this checking by adding a static_assert at the caller (much like you could get the genericity with a helper as you suggested) with a message explaining why this is important, etc.

I don't feel /amazingly/ strongly either way, but I think it's simpler (in implementation and callers) to have the same API & for it to be a no-op for trivially default constructible types.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:601
     this->reserve(N);
-    for (auto I = this->end(), E = this->begin() + N; I != E; ++I)
-      if (ForOverwrite)
-        new (&*I) T;
-      else
+    if (!ForOverwrite || !is_trivially_copyable<T>::value)
+      for (auto I = this->end(), E = this->begin() + N; I != E; ++I)
----------------
Should this be `is_trivially_constructible<T>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115443