[libcxx-commits] [PATCH] D93233: [libc++] Replaces std::sort by Bitset sorting algorithm.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Sorry for the delay, see my answers below.

In D93233#3060110 <https://reviews.llvm.org/D93233#3060110>, @nilayvaish wrote:

> - Fixed the implementation.  No tests fail now: https://buildkite.com/llvm-project/libcxx-ci/builds/5917.
> - Added _LIBCPP_HIDE_FROM_ABI to most functions.
>
> Questions for ldionne@
>
> - How do we test code size?

I think we have benchmarks for `std::sort` -- one thing we could do is look at the size of that executable before and after the change.

Can you also provide the runtime output of these benchmarks before and after the change?

> - Do we need to hide member functions in classes from the ABI?

Yes, but I think you have figured that out already.



================
Comment at: libcxx/include/__algorithm/sort.h:34
 
-// stable, 2-3 compares, 0-2 swaps
+namespace __sorting_network {
 
----------------
Why is this called `__sorting_network`? Sounds like a weird name.


================
Comment at: libcxx/src/legacy-sort.cpp:339-393
+template _LIBCPP_FUNC_VIS void __sort<__less<char>&, char*>(char*, char*, __less<char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*, __less<wchar_t>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<signed char>&, signed char*>(signed char*, signed char*,
+                                                                          __less<signed char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<unsigned char>&, unsigned char*>(unsigned char*, unsigned char*,
+                                                                              __less<unsigned char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<short>&, short*>(short*, short*, __less<short>&);
----------------
nilayvaish wrote:
> ldionne@, I am wondering if these symbols need to be in this file.  Can we continue with the setup from before i.e. these symbols have extern declarations in sort.h and are defined in algorithm.cpp?  Further is there a need for retaining the existing sorting algorithm?  It seems to me we need to retain __insertion_sort_incomplete only.  What do you think?
I think we do need to retain all those functions since they were previously exported from the shared library. Removing these functions would be an ABI break.

I've put it in `legacy-sort.cpp` because we don't ever want to deal with these functions again anymore -- they are only there for ABI compatibility purpose. I thought it was better to separate them into their own little file than keeping them around in the headers that we actually use. If you have a strong reason to keep them around in `sort.h`, let me know and we can discuss.


================
Comment at: libcxx/src/legacy-sort.cpp:340
+template _LIBCPP_FUNC_VIS void __sort<__less<char>&, char*>(char*, char*, __less<char>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*, __less<wchar_t>&);
+template _LIBCPP_FUNC_VIS void __sort<__less<signed char>&, signed char*>(signed char*, signed char*,
----------------
This should be guarded by `_LIBCPP_HAS_NO_WIDE_CHARACTERS`. I think this must have happened when you (or I) rebased the patch.


================
Comment at: libcxx/src/legacy-sort.cpp:363-364
+template _LIBCPP_FUNC_VIS bool __insertion_sort_incomplete<__less<char>&, char*>(char*, char*, __less<char>&);
+template _LIBCPP_FUNC_VIS bool __insertion_sort_incomplete<__less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*,
+                                                                                       __less<wchar_t>&);
+template _LIBCPP_FUNC_VIS bool
----------------
Same.


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

https://reviews.llvm.org/D93233