This is an archive of the discontinued LLVM Phabricator instance.

P1144 "Trivially relocatable" (1/3): is_trivially_relocatable, relocate_at, and uninitialized_relocate
AbandonedPublic

Authored by philnik on May 9 2019, 2:32 PM.

Details

Reviewers
ldionne
mclow.lists
EricWF
Quuxplusone
Group Reviewers
Restricted Project
Summary

Part 0/3 is D50119: it adds __has_extension(__is_trivially_relocatable) to the compiler.
Part 1/3 is D61761: it implements the library traits and algorithms mandated by P1144.
Part 2/3 is D63620: it implements Quality-of-Implementation features to warrant certain std library types as "trivially relocatable."
Part 3/3 is D67524: it implements Quality-of-Implementation features to optimize certain std library functions for better performance on types that have been warranted "trivially relocatable."

This implements the library behavior proposed in P1144R3. This patch doesn't do any special optimizations (e.g. to std::vector::reserve) nor add warrants to libc++ class types; those will come later. This is just the bare minimum to conform to P1144's additions to the standard.

We mangle relocate_at into __libcpp_relocate_at, and so on, in order to discourage users from using them. (The intent is that libc++ itself will be able to use them to enable optimizations. Such optimizations are the point of D67524.)

TODO:

  • any reason to gate the new names on any particular _LIBCPP_STD_VER, such as 17? (I think not.)
  • do the tests pass with a compiler that doesn't support __has_extension(__is_trivially_relocatable)? how best to structure the tests so as not to break this?

Diff Detail

Event Timeline

Quuxplusone created this revision.May 9 2019, 2:32 PM
Quuxplusone edited the summary of this revision. (Show Details)May 9 2019, 2:36 PM
jfb added a subscriber: rjmccall.
zoecarver added inline comments.
include/memory
3463 ↗(On Diff #198907)

Nit: is _IsMemcpyable guaranteed to be true_type or false_type? If so, maybe make it specifically false_type.

3510 ↗(On Diff #198907)

I think this should require _LIBCPP_STD_VER > 03.

3556 ↗(On Diff #198907)

What is the reason for is_volatile? I don't see that in the paper (but maybe I am missing something).

test/libcxx/type_traits/is_trivially_relocatable.pass.cpp
33 ↗(On Diff #198907)

Maybe add a test for things that are experimental?

test/std/utilities/meta/meta.unary/meta.unary.prop/is_trivially_relocatable.pass.cpp
14 ↗(On Diff #198907)

Shouldn't this also contain c++98?

Quuxplusone marked 5 inline comments as done.May 11 2019, 4:54 PM
Quuxplusone added inline comments.
include/memory
3463 ↗(On Diff #198907)

Brain fart on my part. (This isn't a class template with a partial specialization, it's two overloaded function templates!) Will fix.

3466 ↗(On Diff #198907)

I'm not sure whether this launder is doing anything.
Alternatively, should I assign __dest = ::new (... and then return __dest?

3510 ↗(On Diff #198907)

libc++ supports std::move even in _LIBCPP_HAS_NO_RVALUE_REFERENCES-mode, presumably because it would be too tedious to #ifdef all the places where we use std::move (such as here).

3556 ↗(On Diff #198907)

It's related to https://quuxplusone.github.io/blog/2018/07/13/trivially-copyable-corner-cases/ — if we have a range of volatile Foo, then the user is probably expecting us to follow the abstract machine and copy the Foo objects one by one, instead of byte-by-byte (possible tearing). The library doesn't technically have to follow the abstract machine here, but I think it's QoI to do so.

I just re-examined std::copy and it doesn't bother to check is_volatile, so I guess it would be consistent for libc++ not to check is_volatile here either. (That would make the QoI consistently low, IMHO.)

test/std/utilities/meta/meta.unary/meta.unary.prop/is_trivially_relocatable.pass.cpp
14 ↗(On Diff #198907)

Hmm. My impression had been that c++98 and c++03 were synonymous, and that the tests only ever passed c++03. But now I see that most tests do refer to c++98, c++03, except for the ones I added and also

test/std/diagnostics/syserr/is_error_code_enum.pass.cpp
test/std/input.output/iostreams.base/is_error_code_enum_io_errc.pass.cpp
test/std/utilities/time/time.duration/time.duration.alg/abs.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/ceil.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/floor.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/round.fail.cpp
test/std/utilities/time/time.point/time.point.cast/ceil.fail.cpp
test/std/utilities/time/time.point/time.point.cast/floor.fail.cpp
test/std/utilities/time/time.point/time.point.cast/round.fail.cpp

Should these other tests also get c++98 added, or should c++98 and c++03 be synonymous in tests?

Fixed brain fart. Added UNSUPPORTED: c++98 to test.

zoecarver added inline comments.May 12 2019, 11:01 AM
include/memory
3466 ↗(On Diff #198907)

That seems more correct. After calling new the pointer should still be active, so you won't need launder .

I think launder is only necessary after memmove because then the pointer's lifetime is over.

3510 ↗(On Diff #198907)

Good to know.

3556 ↗(On Diff #198907)

Maybe open an issue about this (in bugzilla) so that once/if P1153 is introduced it can be updated.

3464 ↗(On Diff #199160)

Are you intentionally not doing anything with the result of new? Also, I am not sure you need to static_cast to void*.

Also, also, I think you want to move __source not __dest (because I am pretty sure __dest is where you want to relocate the new thing and __source is where you want to get it).

3481 ↗(On Diff #199160)

I think this should be is_trivially_relocatable<_Tp>().

zoecarver added inline comments.May 12 2019, 11:25 AM
include/memory
3518 ↗(On Diff #199160)

I don't think this overload is necessary in addition to the below one.

Quuxplusone marked 10 inline comments as done.

Add a heterogeneous __relocate_at2 helper for uninitialized_relocate.
Also fix a major typo-bug in relocate_at; thanks @zoecarver!

include/memory
3464 ↗(On Diff #199160)

Yikes! That was a pretty bad bug. Thanks.

3518 ↗(On Diff #199160)

I think this overload would never really be useful in practice, but since it does something different from the below one (element-wise memcpy in a loop, instead of one big memcpy), I figured I should implement it.

However, since P1144R3 adds relocate_at, I can refactor this overload and the one above into a single overload that does relocate_at-in-a-loop! Will do.

In the process of rewriting this code, I realized that whereas uninitialized_relocate copies uninitialized_move in not requiring the two iterator types to have the same value_type, relocate_at requires the exact same type _Tp * for both __source and __dest. I have added an internal version __relocate_at2(_Sp *__source, _Dp *__dest), and am ambivalently considering adding it to the proposal P1144R4. That would require some kind of change to the word "homogeneous" in P1144R3, though. Feel free to comment on this idea here and/or by email.

zoecarver added inline comments.May 12 2019, 8:10 PM
include/memory
3464 ↗(On Diff #199160)

Of course! That's what code review is for :)

Maybe add a test for this though.

3518 ↗(On Diff #199160)

IMVHO I think it would be better to have them be one type and let the user static_cast the pointer if they want something different.

If the difference is meaningful, maybe another overload should be added in P1144R3. Otherwise, I think combining them and using relocate_at is a good idea.

Quuxplusone marked an inline comment as done.May 12 2019, 10:16 PM
Quuxplusone added inline comments.
include/memory
3518 ↗(On Diff #199160)

I think I don't understand what you thought I meant. :)
By "homogeneous" relocate_at, I mean it can only relocate from T to that same type T (not even with different cv-qualifiers). As opposed to uninitialized_relocate, which is heterogeneous just like uninitialized_move and std::copy. For example:

long *src;
double *dest;
std::uninitialized_relocate(src, src+n, dest);  // OK
std::relocate_at(src, dest);  // not OK, because the types differ
std::__relocate_at2(src, dest);  // OK

The above case uses a non-trivial relocation operation.

int *src;
std::unique_ptr<int> *dest;
std::uninitialized_relocate(src, src+n, dest);  // OK
std::relocate_at(src, dest);  // not OK, because the types differ
std::__relocate_at2(src, dest);  // OK

The above case is functionally equivalent to memcpy, although this patch won't go so far as to actually make it use memcpy.

zoecarver added inline comments.May 13 2019, 11:28 AM
include/memory
3518 ↗(On Diff #199160)

I understand what you meant. I think it is important that (for the most part) pointer functions are "homogeneous", especially the ones that rely on sizeof.

Here is an example of what I mean:

template<class T>
void cpy(T* dst, T* src)
{
	std::memcpy(dst, src, sizeof(T));
}

int main()
{
	int x = 42;
	int* iptr = &x;
	void* vptr = &x;
	cpy(iptr, vptr); // fails
	cpy(iptr, static_cast<int*>(vptr)); // works
}

The above function will prevent easy run time out of bounds errors from differently sized types.

Context for other reviewers:

I spoke to Arthur at C++Now after his talk, and my opinion was that it would be useful to implement the trivially relocatable proposal as a conforming extension to libc++ to allow gathering some usage experience. However, I am dead against making any change that is user visible. So what I would like is for us to implement the traits and algorithms but use reserved identifiers -- this way we can implement optimizations inside the library without changing the user-visible API. And if the proposal never makes it into C++, we could even remove everything that has to do with trivially relocatable since it was only an internal optimization anyway -- that's the spirit.

Y'all know I'm against extensions, however I think gathering usage experience is useful and this is an opportunity to do so without making user-visible changes, which is one case where I think extensions are easy to sell. Please LMK if you disagree, otherwise I told Arthur I would help shepherd the patch through (the libc++ part).

  • hide names? how?

Use __libcpp_xxx or whatever scheme you like. Those are implementation details anyway, we can change them as we see fit.

  • any reason to gate the new names on any particular _LIBCPP_STD_VER, such as 17? (I think not.)

Good question. On the one hand, you want to get as much usage experience as possible, so that would mean enabling it on the oldest possible standard. On the other hand, once (if) the proposal makes it into C++, we won't want to implement the functionality in older standards. But removing the optimizations from C++03 will cause regressions, which is bad.

  • split up the tests for uninitialized_relocate{,_n} into three or four files each?

The tests look reasonable to me as-is. However, they should all be in test/libcxx/ because they are libc++ specific.

  • write a test for relocate_at

Please do.

ldionne requested changes to this revision.May 14 2019, 1:59 PM

Requesting change so it shows up properly in my review queue.

This revision now requires changes to proceed.May 14 2019, 1:59 PM
Quuxplusone edited the summary of this revision. (Show Details)
  • Hide all the P1144 names consistently behind a __libcpp_ prefix.
  • Add a unit test for __libcpp_relocate_at.
  • Use remove_all_extents in the fallback implementation of is_trivially_relocatable<T>, so that is_trivially_relocatable_v<int[10]> will be true. (is_trivially_relocatable_v<int[]> currently has UB according to P1144, because int[] is an incomplete type.)
Quuxplusone marked 2 inline comments as done.Jun 20 2019, 4:24 PM

@ldionne I've adopted your request to hide all the new names behind __libcpp_.
I think I still need some guidance on how to structure the tests so that they pass with a D50119-enabled Clang and {either pass or are XFAILed} for other compilers.

Rebased on master. (I don't think I've made any changes since last time.)

Rebased on master.

Migrate to the monorepo.
@ldionne @EricWF ping?

Herald added a project: Restricted Project. · View Herald TranscriptJan 1 2020, 4:49 PM
ldionne added a reviewer: Restricted Project.Nov 2 2020, 2:15 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptNov 2 2020, 2:15 PM
philnik commandeered this revision.Aug 31 2023, 5:28 PM
philnik added a reviewer: Quuxplusone.
philnik added a subscriber: philnik.

[Github PR transition cleanup]

I'm commandeering and abandoning this, since this has been stale for quite a while and we probably won't implement it in this way unless it becomes standard. (Also, Arthur probably won't work on it)

Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 5:28 PM
philnik abandoned this revision.Aug 31 2023, 5:29 PM