[PATCH] D115443: [ADT] Weaken the initialization condition in SmallVector::resize_for_overwrite
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 18 16:40:22 PST 2022
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
More information about the llvm-commits
mailing list