This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [P0879] constexpr std::sort, and add new tests for it
ClosedPublic

Authored by Quuxplusone on Dec 21 2020, 12:54 PM.

Details

Reviewers
ldionne
jdoerfert
Group Reviewers
Restricted Project
Commits
rG493f1407927c: [libc++] [P0879] constexpr std::sort
Summary

This completes libc++'s implementation of P0879 "Constexpr for swap and swap related functions."
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0879r0.html

For the feature-macro adjustment, see
https://cplusplus.github.io/LWG/issue3256

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 21 2020, 12:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
libcxx/include/algorithm
3953

This comment was never correct. _Compare may be Pred&, or it may be __debug_less<Pred> without the &.

4147

I'm preserving the old code's strategy here. As I say in D92190, I'd like to change this to call _VSTD::sort(first, last, __less<void*>()) and explicitly instantiate __sort<,__less<void*>&, void*> in "libc++.a"; but I'd like to do that in a separate PR because it has ABI implications.

I also note in passing that it would be awesome if we had a way to detect contiguous iterators in C++17-and-earlier, so that we could use that, instead of special-casing T** and __wrap_iter<T*>. This is related to the use of __unwrap_iter in std::copy; whatever we do should be applied consistently in both sort and copy.

5364

In the nth_element review (D93557) I decided to constexpr-ify the algorithm itself. In this one, I decided that the simplest course of action would be to switch on is_constant_evaluated and if we're doing it at compile-time let's just do a plain old heapsort because that's already constexpr-friendly, and then we don't have to worry about making __sort itself constexpr.

So the name here might be misleading, and I'd take suggestions for better names. Here __sort_constexpr means "This algorithm is guaranteed constexpr-friendly," but it's above the switch-dispatch-thingie, whereas in e.g. __copy_constexpr it's below the switch-dispatch-thingie.

I don't want to change the name of __sort itself in this specific patch, because that would have ABI implications (some explicit template instantiations are placed in "libc++.a"). I'm excited for the prospect of ABI breakage ;) but I'd like to confine any ABI breakage to a separate PR.

Quuxplusone edited the summary of this revision. (Show Details)

Add feature-macro stuff and update the CSV files.

ldionne requested changes to this revision.Jan 25 2021, 9:10 AM

What's the status of this? Can you please rebase so CI runs (there were some CI issues during the holidays, I remember patch application failing). Ping me when this is ready for review, until then I'm marking as "changes requested".

This revision now requires changes to proceed.Jan 25 2021, 9:10 AM

Rebase on D93557, and indicate that we're now targeting release 13.0 not 12.0.

ldionne requested changes to this revision.Jan 28 2021, 8:02 AM
ldionne added inline comments.
libcxx/include/algorithm
5245

I don't think we need this declaration. Isn't it the same as the definition on line 5165?

5364

If we get rid of the __unwrap_iter overload for sort(), we can simply remove __sort_constexpr and that solves the question. WDYT?

5502

In a second step, couldn't we remove this overload to replace it by calls to __unwrap_iter?

This revision now requires changes to proceed.Jan 28 2021, 8:02 AM
Quuxplusone marked an inline comment as done.Jan 28 2021, 9:49 AM
Quuxplusone added inline comments.
libcxx/include/algorithm
5502

As of current main, no, because __unwrap_iter isn't optimized for non-trivially-copyable element types (it unwraps __wrap_iter<int*> but not __wrap_iter<string*>). If we land D94807 first, then yes.

I suppose this implies that I should make D94807 a parent of this one and prioritize it. I think I agree that that would be a much nicer overall solution than the current soup of overload sets.

Temporarily upload the combined diff for D94807 (contiguous iterators) and D93661 (std::sort) together, to see what buildkite thinks of this whole idea.

Quuxplusone updated this revision to Diff 321212.EditedFeb 3 2021, 1:31 PM

Rebase on main (after landing D94807)

At least on OSX, this patch has no effect on the output of nm libc++.dylib or ls -l libc++.dylib. The effect on libc++.a is that it shrinks from 7968'560 to 7968'408 bytes; the diff in nm libc++.a | sort is this, which I don't see why this should happen at all, but anyway it's not a bad thing:

 000000000001bfc0 T __ZNSt3__16locale5__impC2ERKS1_RKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEi
 000000000001c6c0 T __ZNSt3__16locale5__impC1ERKS1_RKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEi
 000000000001c6d0 T __ZNSt3__16locale5__impC2ERKS1_S3_i
-000000000001d188 b __ZZNSt3__117iostream_categoryEvE1s
-000000000001d190 b __ZGVZNSt3__117iostream_categoryEvE1s
-000000000001d198 b __ZZNSt3__18ios_base5iwordEiE5error
-000000000001d1a0 b __ZZNSt3__18ios_base5pwordEiE5error
-000000000001d1a8 S __ZNSt3__18ios_base9__xindex_E
+000000000001d180 b __ZZNSt3__117iostream_categoryEvE1s
+000000000001d188 b __ZGVZNSt3__117iostream_categoryEvE1s
+000000000001d190 b __ZZNSt3__18ios_base5iwordEiE5error
+000000000001d198 b __ZZNSt3__18ios_base5pwordEiE5error
+000000000001d1a0 S __ZNSt3__18ios_base9__xindex_E
 000000000001d4e8 b __ZZNSt3__115future_categoryEvE3__f
 000000000001d4f0 b __ZGVZNSt3__115future_categoryEvE3__f
 000000000001e430 T __ZNSt3__16locale5__impC1ERKS1_S3_i
 000000000001e440 T __ZNSt3__16locale5__impC2ERKS1_PNS0_5facetEl
ldionne accepted this revision.Feb 3 2021, 2:33 PM

LGTM pending CI!

This revision is now accepted and ready to land.Feb 3 2021, 2:33 PM
This revision was automatically updated to reflect the committed changes.
danlark added a subscriber: danlark.Feb 4 2021, 9:28 AM
danlark added inline comments.
libcxx/include/algorithm
5364

One thing to think about:

Yet, std::sort does not guarantee the order of equal elements. With the current patch it may happen that constexpr std::sort and usual std::sort return different ranges because of different stability guarantees and that might be not user friendly, i.e. the property of determinism is lost.

I don't have a strong opinion on that question, however, I'd prefer constexprify this function instead of calling partial_sort

Quuxplusone marked 2 inline comments as done.Feb 4 2021, 11:12 AM
Quuxplusone added inline comments.
libcxx/include/algorithm
5364

With the current patch it may happen that constexpr std::sort and usual std::sort return different ranges [...] i.e. the property of determinism is lost.

That's a good point I hadn't thought of: Maybe constexpr sort produces (1, 2a, 2c, 2b, 3) while runtime sort produces (1, 2b, 2a, 2c, 3). However, arguably, libc++ doesn't want to be in the business of making guarantees about sort's behavior/stability — the user should just avoid writing code that relies on those outcomes.

Vice versa, if we make the existing sort algorithm constexpr-friendly in order to make users happy, I think users would even more so want us to do it without altering any of its existing outcomes. They might object if libc++ v12.0 sort produces (1, 2b, 2a, 2c, 3) [at runtime only] while libc++ v13.0 sort produces (1, 2b, 2c, 2a, 3) [at runtime and compile-time].

danlark added inline comments.Feb 4 2021, 11:27 AM
libcxx/include/algorithm
5364

Depending on the exact values is definitely the user's problem. I don't know if sorting algorithms must be deterministic from the input data but I definitely see cases where some users might expose constant array and check for sorting equality during the runtime

Many questions and unfortunately standard does not answer them fully AFAIK. The algorithm should definitely be deterministic but does changing the evaluation context counts as input?