Apply review comments

Author: Voultapher

libc/src/stdlib/qsort.cpp

@@ -18,11 +18,10 @@ namespace LIBC_NAMESPACE_DECL {
 LLVM_LIBC_FUNCTION(void, qsort,
                    (void *array, size_t array_size, size_t elem_size,
                     int (*compare)(const void *, const void *))) {
-  internal::unstable_sort(
-      array, array_size, elem_size,
-      [compare](const void *a, const void *b) noexcept -> bool {
-        return compare(a, b) < 0;
-      });
+  internal::unstable_sort(array, array_size, elem_size,
+                          [compare](const void *a, const void *b) -> bool {
+                            return compare(a, b) < 0;
+                          });
 }
 
 } // namespace LIBC_NAMESPACE_DECL

libc/src/stdlib/qsort_data.h

@@ -19,47 +19,25 @@
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-// Returns the max amount of bytes deemed reasonable - based on the target
-// properties - for use in local stack arrays.
-constexpr size_t max_stack_array_size() {
-  // Uses target pointer size as heuristic how much memory is available and
-  // unlikely to run into stack overflow and perf problems.
-  constexpr size_t ptr_diff_size = sizeof(ptrdiff_t);
-
-  if constexpr (ptr_diff_size >= 8) {
-    return 4096;
-  }
-
-  if constexpr (ptr_diff_size == 4) {
-    return 512;
-  }
-
-  // 8-bit platforms are just not gonna work well with libc, qsort
-  // won't be the problem.
-  // 16-bit platforms ought to be able to store 64 bytes on the stack.
-  return 64;
-}
-
 class ArrayGenericSize {
-  uint8_t *array_base;
+  cpp::byte *array_base;
   size_t array_len;
   size_t elem_size;
 
-  uint8_t *get_internal(size_t i) const noexcept {
+  LIBC_INLINE cpp::byte *get_internal(size_t i) const {
     return array_base + (i * elem_size);
   }
 
 public:
-  ArrayGenericSize(uint8_t *a, size_t s, size_t e) noexcept
-      : array_base(a), array_len(s), elem_size(e) {}
+  ArrayGenericSize(void *a, size_t s, size_t e)
+      : array_base(reinterpret_cast<cpp::byte *>(a)), array_len(s),
+        elem_size(e) {}
 
   static constexpr bool has_fixed_size() { return false; }
 
-  void *get(size_t i) const noexcept {
-    return reinterpret_cast<void *>(get_internal(i));
-  }
+  LIBC_INLINE void *get(size_t i) const { return get_internal(i); }
 
-  void swap(size_t i, size_t j) const noexcept {
+  void swap(size_t i, size_t j) const {
     // It's possible to use 8 byte blocks with `uint64_t`, but that
     // generates more machine code as the remainder loop gets
     // unrolled, plus 4 byte operations are more likely to be
@@ -70,8 +48,8 @@ class ArrayGenericSize {
     using block_t = uint32_t;
     constexpr size_t BLOCK_SIZE = sizeof(block_t);
 
-    uint8_t *elem_i = get_internal(i);
-    uint8_t *elem_j = get_internal(j);
+    cpp::byte *elem_i = get_internal(i);
+    cpp::byte *elem_j = get_internal(j);
 
     const size_t elem_size_rem = elem_size % BLOCK_SIZE;
     const block_t *elem_i_block_end =
@@ -88,74 +66,73 @@ class ArrayGenericSize {
       elem_j_block += 1;
     }
 
-    elem_i = reinterpret_cast<uint8_t *>(elem_i_block);
-    elem_j = reinterpret_cast<uint8_t *>(elem_j_block);
+    elem_i = reinterpret_cast<cpp::byte *>(elem_i_block);
+    elem_j = reinterpret_cast<cpp::byte *>(elem_j_block);
     for (size_t n = 0; n < elem_size_rem; ++n) {
-      uint8_t tmp = elem_i[n];
+      cpp::byte tmp = elem_i[n];
       elem_i[n] = elem_j[n];
       elem_j[n] = tmp;
     }
   }
 
-  size_t len() const noexcept { return array_len; }
+  LIBC_INLINE size_t len() const { return array_len; }
 
   // Make an Array starting at index |i| and length |s|.
-  ArrayGenericSize make_array(size_t i, size_t s) const noexcept {
+  LIBC_INLINE ArrayGenericSize make_array(size_t i, size_t s) const {
     return ArrayGenericSize(get_internal(i), s, elem_size);
   }
 
   // Reset this Array to point at a different interval of the same
   // items starting at index |i|.
-  void reset_bounds(size_t i, size_t s) noexcept {
+  LIBC_INLINE void reset_bounds(size_t i, size_t s) {
     array_base = get_internal(i);
     array_len = s;
   }
 };
 
-// Having a specialized Array type for sorting that knowns at
+// Having a specialized Array type for sorting that knows at
 // compile-time what the size of the element is, allows for much more
 // efficient swapping and for cheaper offset calculations.
 template <size_t ELEM_SIZE> class ArrayFixedSize {
-  uint8_t *array_base;
+  cpp::byte *array_base;
   size_t array_len;
 
-  uint8_t *get_internal(size_t i) const noexcept {
+  LIBC_INLINE cpp::byte *get_internal(size_t i) const {
     return array_base + (i * ELEM_SIZE);
   }
 
 public:
-  ArrayFixedSize(uint8_t *a, size_t s) noexcept : array_base(a), array_len(s) {}
+  ArrayFixedSize(void *a, size_t s)
+      : array_base(reinterpret_cast<cpp::byte *>(a)), array_len(s) {}
 
   // Beware this function is used a heuristic for cheap to swap types, so
   // instantiating `ArrayFixedSize` with `ELEM_SIZE > 100` is probably a bad
   // idea perf wise.
   static constexpr bool has_fixed_size() { return true; }
 
-  void *get(size_t i) const noexcept {
-    return reinterpret_cast<void *>(get_internal(i));
-  }
+  LIBC_INLINE void *get(size_t i) const { return get_internal(i); }
 
-  void swap(size_t i, size_t j) const noexcept {
-    alignas(32) uint8_t tmp[ELEM_SIZE];
+  LIBC_INLINE void swap(size_t i, size_t j) const {
+    alignas(32) cpp::byte tmp[ELEM_SIZE];
 
-    uint8_t *elem_i = get_internal(i);
-    uint8_t *elem_j = get_internal(j);
+    cpp::byte *elem_i = get_internal(i);
+    cpp::byte *elem_j = get_internal(j);
 
     inline_memcpy(tmp, elem_i, ELEM_SIZE);
     inline_memmove(elem_i, elem_j, ELEM_SIZE);
     inline_memcpy(elem_j, tmp, ELEM_SIZE);
   }
 
-  size_t len() const noexcept { return array_len; }
+  LIBC_INLINE size_t len() const { return array_len; }
 
   // Make an Array starting at index |i| and length |s|.
-  ArrayFixedSize<ELEM_SIZE> make_array(size_t i, size_t s) const noexcept {
+  LIBC_INLINE ArrayFixedSize<ELEM_SIZE> make_array(size_t i, size_t s) const {
     return ArrayFixedSize<ELEM_SIZE>(get_internal(i), s);
   }
 
   // Reset this Array to point at a different interval of the same
   // items starting at index |i|.
-  void reset_bounds(size_t i, size_t s) noexcept {
+  LIBC_INLINE void reset_bounds(size_t i, size_t s) {
     array_base = get_internal(i);
     array_len = s;
   }

libc/src/stdlib/qsort_pivot.h

@@ -9,8 +9,6 @@
 #ifndef LLVM_LIBC_SRC_STDLIB_QSORT_PIVOT_H
 #define LLVM_LIBC_SRC_STDLIB_QSORT_PIVOT_H
 
-#include "src/stdlib/qsort_pivot.h"
-
 #include <stdint.h>
 
 namespace LIBC_NAMESPACE_DECL {
@@ -75,11 +73,10 @@ size_t median3(const A &array, size_t a, size_t b, size_t c, const F &is_less) {
     // If x=y=1 then a < b, c. In this case we want to return min(b, c).
     // By toggling the outcome of b < c using XOR x we get this behavior.
     const bool z = is_less(b_ptr, c_ptr);
-    if (z ^ x) {
+    if (z ^ x)
       return c;
-    } else {
+    else
       return b;
-    }
   } else {
     // Either c <= a < b or b <= a < c, thus a is our median.
     return a;

libc/src/stdlib/qsort_r.cpp

@@ -20,11 +20,10 @@ LLVM_LIBC_FUNCTION(void, qsort_r,
                     int (*compare)(const void *, const void *, void *),
                     void *arg)) {
 
-  internal::unstable_sort(
-      array, array_size, elem_size,
-      [compare, arg](const void *a, const void *b) noexcept -> bool {
-        return compare(a, b, arg) < 0;
-      });
+  internal::unstable_sort(array, array_size, elem_size,
+                          [compare, arg](const void *a, const void *b) -> bool {
+                            return compare(a, b, arg) < 0;
+                          });
 }
 
 } // namespace LIBC_NAMESPACE_DECL

libc/src/stdlib/qsort_util.h

@@ -15,9 +15,13 @@
 #define LIBC_QSORT_QUICK_SORT 1
 #define LIBC_QSORT_HEAP_SORT 2
 
+#ifdef LIBC_OPTIMIZE_FOR_SIZE
+#define LIBC_QSORT_IMPL LIBC_QSORT_HEAP_SORT
+#else
 #ifndef LIBC_QSORT_IMPL
 #define LIBC_QSORT_IMPL LIBC_QSORT_QUICK_SORT
 #endif // LIBC_QSORT_IMPL
+#endif // LIBC_OPTIMIZE_FOR_SIZE
 
 #if (LIBC_QSORT_IMPL != LIBC_QSORT_QUICK_SORT &&                               \
      LIBC_QSORT_IMPL != LIBC_QSORT_HEAP_SORT)
@@ -27,45 +31,50 @@
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-template <typename A, typename F> void sort_inst(A &array, const F &is_less) {
-#if LIBC_QSORT_IMPL == LIBC_QSORT_QUICK_SORT
-  quick_sort(array, is_less);
-#elif LIBC_QSORT_IMPL == LIBC_QSORT_HEAP_SORT
-  heap_sort(array, is_less);
-#endif
+template <bool USE_QUICKSORT, typename F>
+void unstable_sort_impl(void *array, size_t array_len, size_t elem_size,
+                        const F &is_less) {
+  if (array == nullptr || array_len == 0 || elem_size == 0)
+    return;
+
+  if constexpr (USE_QUICKSORT) {
+    switch (elem_size) {
+    case 4: {
+      auto arr_fixed_size = internal::ArrayFixedSize<4>(array, array_len);
+      quick_sort(arr_fixed_size, is_less);
+      return;
+    }
+    case 8: {
+      auto arr_fixed_size = internal::ArrayFixedSize<8>(array, array_len);
+      quick_sort(arr_fixed_size, is_less);
+      return;
+    }
+    case 16: {
+      auto arr_fixed_size = internal::ArrayFixedSize<16>(array, array_len);
+      quick_sort(arr_fixed_size, is_less);
+      return;
+    }
+    default:
+      auto arr_generic_size =
+          internal::ArrayGenericSize(array, array_len, elem_size);
+      quick_sort(arr_generic_size, is_less);
+      return;
+    }
+  } else {
+    auto arr_generic_size =
+        internal::ArrayGenericSize(array, array_len, elem_size);
+    heap_sort(arr_generic_size, is_less);
+  }
 }
 
 template <typename F>
 void unstable_sort(void *array, size_t array_len, size_t elem_size,
                    const F &is_less) {
-  if (array == nullptr || array_len == 0 || elem_size == 0) {
-    return;
-  }
-
-  uint8_t *array_base = reinterpret_cast<uint8_t *>(array);
-
-  switch (elem_size) {
-  case 4: {
-    auto arr_fixed_size = internal::ArrayFixedSize<4>(array_base, array_len);
-    sort_inst(arr_fixed_size, is_less);
-    return;
-  }
-  case 8: {
-    auto arr_fixed_size = internal::ArrayFixedSize<8>(array_base, array_len);
-    sort_inst(arr_fixed_size, is_less);
-    return;
-  }
-  case 16: {
-    auto arr_fixed_size = internal::ArrayFixedSize<16>(array_base, array_len);
-    sort_inst(arr_fixed_size, is_less);
-    return;
-  }
-  default:
-    auto arr_generic_size =
-        internal::ArrayGenericSize(array_base, array_len, elem_size);
-    sort_inst(arr_generic_size, is_less);
-    return;
-  }
+#if LIBC_QSORT_IMPL == LIBC_QSORT_QUICK_SORT
+  unstable_sort_impl<true, F>(array, array_len, elem_size, is_less);
+#elif LIBC_QSORT_IMPL == LIBC_QSORT_HEAP_SORT
+  unstable_sort_impl<false, F>(array, array_len, elem_size, is_less);
+#endif
 }
 
 } // namespace internal

libc/src/stdlib/quick_sort.h

@@ -9,8 +9,8 @@
 #ifndef LLVM_LIBC_SRC_STDLIB_QUICK_SORT_H
 #define LLVM_LIBC_SRC_STDLIB_QUICK_SORT_H
 
+#include "src/__support/CPP/bit.h"
 #include "src/__support/CPP/cstddef.h"
-#include "src/__support/big_int.h"
 #include "src/__support/macros/config.h"
 #include "src/stdlib/qsort_pivot.h"
 
@@ -90,7 +90,7 @@ size_t partition(const A &array, size_t pivot_index, const F &is_less) {
   size_t num_lt;
   if constexpr (A::has_fixed_size()) {
     // Branchless Lomuto avoid branch misprediction penalties, but
-    // it also swaps more often which only is faster if the swap a
+    // it also swaps more often which is only faster if the swap is a fast
     // constant operation.
     num_lt = partition_lomuto_branchless(array_without_pivot, pivot, is_less);
   } else {
@@ -130,7 +130,7 @@ void quick_sort_impl(A &array, const void *ancestor_pivot, size_t limit,
       if (!is_less(ancestor_pivot, array.get(pivot_index))) {
         const size_t num_lt =
             partition(array, pivot_index,
-                      [is_less](const void *a, const void *b) noexcept -> bool {
+                      [is_less](const void *a, const void *b) -> bool {
                         return !is_less(b, a);
                       });
 

libc/test/src/stdlib/CMakeLists.txt

@@ -313,11 +313,11 @@ add_libc_test(
 )
 
 add_libc_test(
-  qsort_test
+  quick_sort_test
   SUITE
     libc-stdlib-tests
   SRCS
-    qsort_test.cpp
+    quick_sort_test.cpp
   HDRS
     SortingTest.h
   DEPENDS