Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc] Improve qsort (with build fix) #121482

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

Voultapher
Copy link
Contributor

@Voultapher Voultapher commented Jan 2, 2025

Re-open of #120450 with known build issues resolved.

@Voultapher
Copy link
Contributor Author

Voultapher commented Jan 2, 2025

Not sure on the git approach here, should I do the original commits or revert the revert and just add the new commit?

@llvmbot llvmbot added libc bazel "Peripheral" support tier build system: utils/bazel labels Jan 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2025

@llvm/pr-subscribers-libc

Author: Lukas Bergdoll (Voultapher)

Changes

Re-open of #120450 with known build issues resolved.

Tagging @SchrodingerZhu @michaelrj-google.


Patch is 47.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121482.diff

17 Files Affected:

  • (modified) libc/fuzzing/stdlib/CMakeLists.txt (+3-3)
  • (modified) libc/fuzzing/stdlib/heap_sort_fuzz.cpp (+13-16)
  • (renamed) libc/fuzzing/stdlib/quick_sort_fuzz.cpp (+14-15)
  • (modified) libc/src/stdlib/heap_sort.h (+6-6)
  • (modified) libc/src/stdlib/qsort.cpp (+4-6)
  • (modified) libc/src/stdlib/qsort_data.h (+101-70)
  • (added) libc/src/stdlib/qsort_pivot.h (+85)
  • (modified) libc/src/stdlib/qsort_r.cpp (+5-6)
  • (modified) libc/src/stdlib/qsort_util.h (+42-5)
  • (modified) libc/src/stdlib/quick_sort.h (+147-56)
  • (modified) libc/test/src/stdlib/CMakeLists.txt (+3-15)
  • (modified) libc/test/src/stdlib/SortingTest.h (+112-87)
  • (modified) libc/test/src/stdlib/heap_sort_test.cpp (+14-4)
  • (modified) libc/test/src/stdlib/qsort_r_test.cpp (+2-2)
  • (removed) libc/test/src/stdlib/qsort_test.cpp (-17)
  • (modified) libc/test/src/stdlib/quick_sort_test.cpp (+14-5)
  • (modified) utils/bazel/llvm-project-overlay/libc/test/src/stdlib/BUILD.bazel (+4-12)
diff --git a/libc/fuzzing/stdlib/CMakeLists.txt b/libc/fuzzing/stdlib/CMakeLists.txt
index 9b3298cfc55a77..3dbd640a67dbd8 100644
--- a/libc/fuzzing/stdlib/CMakeLists.txt
+++ b/libc/fuzzing/stdlib/CMakeLists.txt
@@ -1,9 +1,9 @@
 add_libc_fuzzer(
-  qsort_fuzz
+  quick_sort_fuzz
   SRCS
-    qsort_fuzz.cpp
+    quick_sort_fuzz.cpp
   DEPENDS
-    libc.src.stdlib.qsort
+    libc.src.stdlib.qsort_util
 )
 
 add_libc_fuzzer(
diff --git a/libc/fuzzing/stdlib/heap_sort_fuzz.cpp b/libc/fuzzing/stdlib/heap_sort_fuzz.cpp
index 876c5f9975d4d9..6b00306ec7dc1d 100644
--- a/libc/fuzzing/stdlib/heap_sort_fuzz.cpp
+++ b/libc/fuzzing/stdlib/heap_sort_fuzz.cpp
@@ -10,21 +10,10 @@
 ///
 //===----------------------------------------------------------------------===//
 
-#include "src/stdlib/heap_sort.h"
+#include "src/stdlib/qsort_util.h"
 #include <stdint.h>
 
-static int int_compare(const void *l, const void *r) {
-  int li = *reinterpret_cast<const int *>(l);
-  int ri = *reinterpret_cast<const int *>(r);
-  if (li == ri)
-    return 0;
-  if (li > ri)
-    return 1;
-  return -1;
-}
-
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
-
   const size_t array_size = size / sizeof(int);
   if (array_size == 0)
     return 0;
@@ -34,14 +23,22 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   for (size_t i = 0; i < array_size; ++i)
     array[i] = data_as_int[i];
 
-  auto arr = LIBC_NAMESPACE::internal::Array(
-      reinterpret_cast<uint8_t *>(array), array_size, sizeof(int), int_compare);
+  const auto is_less = [](const void *a_ptr,
+                          const void *b_ptr) noexcept -> bool {
+    const int &a = *static_cast<const int *>(a_ptr);
+    const int &b = *static_cast<const int *>(b_ptr);
+
+    return a < b;
+  };
 
-  LIBC_NAMESPACE::internal::heap_sort(arr);
+  constexpr bool USE_QUICKSORT = false;
+  LIBC_NAMESPACE::internal::unstable_sort_impl<USE_QUICKSORT>(
+      array, array_size, sizeof(int), is_less);
 
-  for (size_t i = 0; i < array_size - 1; ++i)
+  for (size_t i = 0; i < array_size - 1; ++i) {
     if (array[i] > array[i + 1])
       __builtin_trap();
+  }
 
   delete[] array;
   return 0;
diff --git a/libc/fuzzing/stdlib/qsort_fuzz.cpp b/libc/fuzzing/stdlib/quick_sort_fuzz.cpp
similarity index 62%
rename from libc/fuzzing/stdlib/qsort_fuzz.cpp
rename to libc/fuzzing/stdlib/quick_sort_fuzz.cpp
index 5d5053cff5c58c..6371e851d2fc3e 100644
--- a/libc/fuzzing/stdlib/qsort_fuzz.cpp
+++ b/libc/fuzzing/stdlib/quick_sort_fuzz.cpp
@@ -1,4 +1,4 @@
-//===-- qsort_fuzz.cpp ----------------------------------------------------===//
+//===-- quick_sort_fuzz.cpp------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,24 +6,13 @@
 //
 //===----------------------------------------------------------------------===//
 ///
-/// Fuzzing test for llvm-libc qsort implementation.
+/// Fuzzing test for llvm-libc quick_sort implementation.
 ///
 //===----------------------------------------------------------------------===//
 
-#include "src/stdlib/qsort.h"
+#include "src/stdlib/qsort_util.h"
 #include <stdint.h>
 
-static int int_compare(const void *l, const void *r) {
-  int li = *reinterpret_cast<const int *>(l);
-  int ri = *reinterpret_cast<const int *>(r);
-  if (li == ri)
-    return 0;
-  else if (li > ri)
-    return 1;
-  else
-    return -1;
-}
-
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   const size_t array_size = size / sizeof(int);
   if (array_size == 0)
@@ -34,7 +23,17 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   for (size_t i = 0; i < array_size; ++i)
     array[i] = data_as_int[i];
 
-  LIBC_NAMESPACE::qsort(array, array_size, sizeof(int), int_compare);
+  const auto is_less = [](const void *a_ptr,
+                          const void *b_ptr) noexcept -> bool {
+    const int &a = *static_cast<const int *>(a_ptr);
+    const int &b = *static_cast<const int *>(b_ptr);
+
+    return a < b;
+  };
+
+  constexpr bool USE_QUICKSORT = true;
+  LIBC_NAMESPACE::internal::unstable_sort_impl<USE_QUICKSORT>(
+      array, array_size, sizeof(int), is_less);
 
   for (size_t i = 0; i < array_size - 1; ++i) {
     if (array[i] > array[i + 1])
diff --git a/libc/src/stdlib/heap_sort.h b/libc/src/stdlib/heap_sort.h
index ccb9ec5f82149e..b9699776df89c1 100644
--- a/libc/src/stdlib/heap_sort.h
+++ b/libc/src/stdlib/heap_sort.h
@@ -18,11 +18,12 @@ namespace internal {
 // A simple in-place heapsort implementation.
 // Follow the implementation in https://en.wikipedia.org/wiki/Heapsort.
 
-LIBC_INLINE void heap_sort(const Array &array) {
-  size_t end = array.size();
+template <typename A, typename F>
+LIBC_INLINE void heap_sort(const A &array, const F &is_less) {
+  size_t end = array.len();
   size_t start = end / 2;
 
-  auto left_child = [](size_t i) -> size_t { return 2 * i + 1; };
+  const auto left_child = [](size_t i) -> size_t { return 2 * i + 1; };
 
   while (end > 1) {
     if (start > 0) {
@@ -40,12 +41,11 @@ LIBC_INLINE void heap_sort(const Array &array) {
     while (left_child(root) < end) {
       size_t child = left_child(root);
       // If there are two children, set child to the greater.
-      if (child + 1 < end &&
-          array.elem_compare(child, array.get(child + 1)) < 0)
+      if ((child + 1 < end) && is_less(array.get(child), array.get(child + 1)))
         ++child;
 
       // If the root is less than the greater child
-      if (array.elem_compare(root, array.get(child)) >= 0)
+      if (!is_less(array.get(root), array.get(child)))
         break;
 
       // Swap the root with the greater child and continue sifting down.
diff --git a/libc/src/stdlib/qsort.cpp b/libc/src/stdlib/qsort.cpp
index 65a63c239f5c0d..0bf5fc79805279 100644
--- a/libc/src/stdlib/qsort.cpp
+++ b/libc/src/stdlib/qsort.cpp
@@ -18,14 +18,12 @@ 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 *))) {
-  if (array == nullptr || array_size == 0 || elem_size == 0)
-    return;
-  internal::Comparator c(compare);
 
-  auto arr = internal::Array(reinterpret_cast<uint8_t *>(array), array_size,
-                             elem_size, c);
+  const auto is_less = [compare](const void *a, const void *b) -> bool {
+    return compare(a, b) < 0;
+  };
 
-  internal::sort(arr);
+  internal::unstable_sort(array, array_size, elem_size, is_less);
 }
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdlib/qsort_data.h b/libc/src/stdlib/qsort_data.h
index c529d55ca46ffd..aa6d9bbc123de8 100644
--- a/libc/src/stdlib/qsort_data.h
+++ b/libc/src/stdlib/qsort_data.h
@@ -17,91 +17,122 @@
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-using Compare = int(const void *, const void *);
-using CompareWithState = int(const void *, const void *, void *);
-
-enum class CompType { COMPARE, COMPARE_WITH_STATE };
-
-struct Comparator {
-  union {
-    Compare *comp_func;
-    CompareWithState *comp_func_r;
-  };
-  const CompType comp_type;
-
-  void *arg;
-
-  Comparator(Compare *func)
-      : comp_func(func), comp_type(CompType::COMPARE), arg(nullptr) {}
-
-  Comparator(CompareWithState *func, void *arg_val)
-      : comp_func_r(func), comp_type(CompType::COMPARE_WITH_STATE),
-        arg(arg_val) {}
-
-#if defined(__clang__)
-  // Recent upstream changes to -fsanitize=function find more instances of
-  // function type mismatches. One case is with the comparator passed to this
-  // class. Libraries will tend to pass comparators that take pointers to
-  // varying types while this comparator expects to accept const void pointers.
-  // Ideally those tools would pass a function that strictly accepts const
-  // void*s to avoid UB, or would use qsort_r to pass their own comparator.
-  [[clang::no_sanitize("function")]]
-#endif
-  int comp_vals(const void *a, const void *b) const {
-    if (comp_type == CompType::COMPARE) {
-      return comp_func(a, b);
-    } else {
-      return comp_func_r(a, b, arg);
+class ArrayGenericSize {
+  cpp::byte *array_base;
+  size_t array_len;
+  size_t elem_size;
+
+  LIBC_INLINE cpp::byte *get_internal(size_t i) const {
+    return array_base + (i * elem_size);
+  }
+
+public:
+  LIBC_INLINE 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; }
+
+  LIBC_INLINE void *get(size_t i) const { return get_internal(i); }
+
+  LIBC_INLINE 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
+    // efficient on a wider variety of hardware. On x86 LLVM tends
+    // to unroll the block loop again into 2 16 byte swaps per
+    // iteration which is another reason that 4 byte blocks yields
+    // good performance even for big types.
+    using block_t = uint32_t;
+    constexpr size_t BLOCK_SIZE = sizeof(block_t);
+
+    alignas(block_t) cpp::byte tmp_block[BLOCK_SIZE];
+
+    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 cpp::byte *elem_i_block_end = elem_i + (elem_size - elem_size_rem);
+
+    while (elem_i != elem_i_block_end) {
+      __builtin_memcpy(tmp_block, elem_i, BLOCK_SIZE);
+      __builtin_memcpy(elem_i, elem_j, BLOCK_SIZE);
+      __builtin_memcpy(elem_j, tmp_block, BLOCK_SIZE);
+
+      elem_i += BLOCK_SIZE;
+      elem_j += BLOCK_SIZE;
+    }
+
+    for (size_t n = 0; n < elem_size_rem; ++n) {
+      cpp::byte tmp = elem_i[n];
+      elem_i[n] = elem_j[n];
+      elem_j[n] = tmp;
     }
   }
+
+  LIBC_INLINE size_t len() const { return array_len; }
+
+  // Make an Array starting at index |i| and length |s|.
+  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|.
+  LIBC_INLINE void reset_bounds(size_t i, size_t s) {
+    array_base = get_internal(i);
+    array_len = s;
+  }
 };
 
-class Array {
-  uint8_t *array;
-  size_t array_size;
-  size_t elem_size;
-  Comparator compare;
+// 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 {
+  cpp::byte *array_base;
+  size_t array_len;
 
-public:
-  Array(uint8_t *a, size_t s, size_t e, Comparator c)
-      : array(a), array_size(s), elem_size(e), compare(c) {}
-
-  uint8_t *get(size_t i) const { return array + i * elem_size; }
-
-  void swap(size_t i, size_t j) const {
-    uint8_t *elem_i = get(i);
-    uint8_t *elem_j = get(j);
-    for (size_t b = 0; b < elem_size; ++b) {
-      uint8_t temp = elem_i[b];
-      elem_i[b] = elem_j[b];
-      elem_j[b] = temp;
-    }
+  LIBC_INLINE cpp::byte *get_internal(size_t i) const {
+    return array_base + (i * ELEM_SIZE);
   }
 
-  int elem_compare(size_t i, const uint8_t *other) const {
-    // An element must compare equal to itself so we don't need to consult the
-    // user provided comparator.
-    if (get(i) == other)
-      return 0;
-    return compare.comp_vals(get(i), other);
+public:
+  LIBC_INLINE 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; }
+
+  LIBC_INLINE void *get(size_t i) const { return get_internal(i); }
+
+  LIBC_INLINE void swap(size_t i, size_t j) const {
+    alignas(32) cpp::byte tmp[ELEM_SIZE];
+
+    cpp::byte *elem_i = get_internal(i);
+    cpp::byte *elem_j = get_internal(j);
+
+    __builtin_memcpy(tmp, elem_i, ELEM_SIZE);
+    __builtin_memmove(elem_i, elem_j, ELEM_SIZE);
+    __builtin_memcpy(elem_j, tmp, ELEM_SIZE);
   }
 
-  size_t size() const { return array_size; }
+  LIBC_INLINE size_t len() const { return array_len; }
 
-  // Make an Array starting at index |i| and size |s|.
-  LIBC_INLINE Array make_array(size_t i, size_t s) const {
-    return Array(get(i), s, elem_size, compare);
+  // Make an Array starting at index |i| and length |s|.
+  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.
-  LIBC_INLINE void reset_bounds(uint8_t *a, size_t s) {
-    array = a;
-    array_size = s;
+  // Reset this Array to point at a different interval of the same
+  // items starting at index |i|.
+  LIBC_INLINE void reset_bounds(size_t i, size_t s) {
+    array_base = get_internal(i);
+    array_len = s;
   }
 };
 
-using SortingRoutine = void(const Array &);
-
 } // namespace internal
 } // namespace LIBC_NAMESPACE_DECL
 
diff --git a/libc/src/stdlib/qsort_pivot.h b/libc/src/stdlib/qsort_pivot.h
new file mode 100644
index 00000000000000..b7e1b4294f6d61
--- /dev/null
+++ b/libc/src/stdlib/qsort_pivot.h
@@ -0,0 +1,85 @@
+//===-- Implementation header for qsort utilities ---------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDLIB_QSORT_PIVOT_H
+#define LLVM_LIBC_SRC_STDLIB_QSORT_PIVOT_H
+
+#include <stdint.h>
+
+namespace LIBC_NAMESPACE_DECL {
+namespace internal {
+
+// Recursively select a pseudomedian if above this threshold.
+constexpr size_t PSEUDO_MEDIAN_REC_THRESHOLD = 64;
+
+// Selects a pivot from `array`. Algorithm taken from glidesort by Orson Peters.
+//
+// This chooses a pivot by sampling an adaptive amount of points, approximating
+// the quality of a median of sqrt(n) elements.
+template <typename A, typename F>
+size_t choose_pivot(const A &array, const F &is_less) {
+  const size_t len = array.len();
+
+  if (len < 8) {
+    return 0;
+  }
+
+  const size_t len_div_8 = len / 8;
+
+  const size_t a = 0;             // [0, floor(n/8))
+  const size_t b = len_div_8 * 4; // [4*floor(n/8), 5*floor(n/8))
+  const size_t c = len_div_8 * 7; // [7*floor(n/8), 8*floor(n/8))
+
+  if (len < PSEUDO_MEDIAN_REC_THRESHOLD)
+    return median3(array, a, b, c, is_less);
+  else
+    return median3_rec(array, a, b, c, len_div_8, is_less);
+}
+
+// Calculates an approximate median of 3 elements from sections a, b, c, or
+// recursively from an approximation of each, if they're large enough. By
+// dividing the size of each section by 8 when recursing we have logarithmic
+// recursion depth and overall sample from f(n) = 3*f(n/8) -> f(n) =
+// O(n^(log(3)/log(8))) ~= O(n^0.528) elements.
+template <typename A, typename F>
+size_t median3_rec(const A &array, size_t a, size_t b, size_t c, size_t n,
+                   const F &is_less) {
+  if (n * 8 >= PSEUDO_MEDIAN_REC_THRESHOLD) {
+    const size_t n8 = n / 8;
+    a = median3_rec(array, a, a + (n8 * 4), a + (n8 * 7), n8, is_less);
+    b = median3_rec(array, b, b + (n8 * 4), b + (n8 * 7), n8, is_less);
+    c = median3_rec(array, c, c + (n8 * 4), c + (n8 * 7), n8, is_less);
+  }
+  return median3(array, a, b, c, is_less);
+}
+
+/// Calculates the median of 3 elements.
+template <typename A, typename F>
+size_t median3(const A &array, size_t a, size_t b, size_t c, const F &is_less) {
+  const void *a_ptr = array.get(a);
+  const void *b_ptr = array.get(b);
+  const void *c_ptr = array.get(c);
+
+  const bool x = is_less(a_ptr, b_ptr);
+  const bool y = is_less(a_ptr, c_ptr);
+  if (x == y) {
+    // If x=y=0 then b, c <= a. In this case we want to return max(b, c).
+    // 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);
+    return z ^ x ? c : b;
+  } else {
+    // Either c <= a < b or b <= a < c, thus a is our median.
+    return a;
+  }
+}
+
+} // namespace internal
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STDLIB_QSORT_PIVOT_H
diff --git a/libc/src/stdlib/qsort_r.cpp b/libc/src/stdlib/qsort_r.cpp
index bf61a40e847341..4e60998b6a6df9 100644
--- a/libc/src/stdlib/qsort_r.cpp
+++ b/libc/src/stdlib/qsort_r.cpp
@@ -19,13 +19,12 @@ LLVM_LIBC_FUNCTION(void, qsort_r,
                    (void *array, size_t array_size, size_t elem_size,
                     int (*compare)(const void *, const void *, void *),
                     void *arg)) {
-  if (array == nullptr || array_size == 0 || elem_size == 0)
-    return;
-  internal::Comparator c(compare, arg);
-  auto arr = internal::Array(reinterpret_cast<uint8_t *>(array), array_size,
-                             elem_size, c);
 
-  internal::sort(arr);
+  const auto is_less = [compare, arg](const void *a, const void *b) -> bool {
+    return compare(a, b, arg) < 0;
+  };
+
+  internal::unstable_sort(array, array_size, elem_size, is_less);
 }
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdlib/qsort_util.h b/libc/src/stdlib/qsort_util.h
index d42adde06d9762..7882b829d32744 100644
--- a/libc/src/stdlib/qsort_util.h
+++ b/libc/src/stdlib/qsort_util.h
@@ -27,11 +27,48 @@
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-#if LIBC_QSORT_IMPL == LIBC_QSORT_QUICK_SORT
-constexpr auto sort = quick_sort;
-#elif LIBC_QSORT_IMPL == LIBC_QSORT_HEAP_SORT
-constexpr auto sort = heap_sort;
-#endif
+template <bool USE_QUICKSORT, typename F>
+LIBC_INLINE 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>
+LIBC_INLINE void unstable_sort(void *array, size_t array_len, size_t elem_size,
+                               const F &is_less) {
+#define USE_QUICK_SORT ((LIBC_QSORT_IMPL) == (LIBC_QSORT_QUICK_SORT))
+  unstable_sort_impl<USE_QUICK_SORT, F>(array, array_len, elem_size, is_less);
+}
 
 } // namespace internal
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdlib/quick_sort.h b/libc/src/stdlib/quick_sort.h
index 82b90a7d511d99..9ab28302500186 100644
--- a/libc/src/stdlib/quick_sort.h
+++ b/libc/src/stdlib/quick_sort.h
@@ -9,84 +9,175 @@
 #ifndef LLVM_LIBC_SRC_STDLIB_QUICK_SORT_H
 #define LLVM_LIBC_SRC_STDLIB_QUICK_SORT_H
 
-#include "src/__support/macros/attributes.h"
+#include "src/__support/CPP/bit.h"
+#include "src/__support/CPP/cstddef.h"
 #include "src/__support/macros/config.h"
-#include "src/stdlib/qsort_data.h"
+#include "src/stdlib/qsort_pivot.h"
 
 #include <stdint.h>
 
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-// A simple quicksort implementation using the Hoare partition scheme.
-LIBC_INLINE size_t partition(const Array &array) {
-  const size_t array_size = array.size();
-  size...
[truncated]

Copy link
Contributor

@SchrodingerZhu SchrodingerZhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the build!

@SchrodingerZhu
Copy link
Contributor

Not sure on the git approach here, should I do the original commits or revert the revert and just add the new commit?

yes, that would be one way to do it

@SchrodingerZhu SchrodingerZhu merged commit a738d81 into llvm:main Jan 4, 2025
13 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jan 4, 2025

LLVM Buildbot has detected a new failure on builder libc-x86_64-debian-gcc-fullbuild-dbg running on libc-x86_64-debian-fullbuild while building libc,utils at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/13255

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
[ RUN      ] LlvmLibcStrtouint32Test.DecodeInOtherBases
[       OK ] LlvmLibcStrtouint32Test.DecodeInOtherBases (483 ms)
[ RUN      ] LlvmLibcStrtouint32Test.CleanBaseSixteenDecode
[       OK ] LlvmLibcStrtouint32Test.CleanBaseSixteenDecode (66 us)
[ RUN      ] LlvmLibcStrtouint32Test.MessyBaseSixteenDecode
[       OK ] LlvmLibcStrtouint32Test.MessyBaseSixteenDecode (4 us)
[ RUN      ] LlvmLibcStrtouint32Test.AutomaticBaseSelection
[       OK ] LlvmLibcStrtouint32Test.AutomaticBaseSelection (5 us)
Ran 14 tests.  PASS: 14  FAIL: 0
[1105/1114] Building CXX object libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.heap_sort_test.__unit__.__build__.dir/heap_sort_test.cpp.o
FAILED: libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.heap_sort_test.__unit__.__build__.dir/heap_sort_test.cpp.o 
/usr/bin/g++ -DLIBC_NAMESPACE=__llvm_libc_20_0_0_git -D_DEBUG -I/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc -isystem /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/build/libc/include -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wimplicit-fallthrough -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -fpie -DLIBC_FULL_BUILD -ffreestanding -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -fext-numeric-literals -Wno-pedantic -std=gnu++17 -MD -MT libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.heap_sort_test.__unit__.__build__.dir/heap_sort_test.cpp.o -MF libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.heap_sort_test.__unit__.__build__.dir/heap_sort_test.cpp.o.d -o libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.heap_sort_test.__unit__.__build__.dir/heap_sort_test.cpp.o -c /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/heap_sort_test.cpp
In file included from /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/heap_sort_test.cpp:9:
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h: In lambda function:
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h:310:34: error: ‘ARRAY_INITIAL_VALS’ is not captured
  310 |         const uint8_t elem_val = ARRAY_INITIAL_VALS[elem_i];
      |                                  ^~~~~~~~~~~~~~~~~~
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h:304:32: note: the lambda has no capture-default
  304 |     const auto fill_buf = [&buf](size_t elem_size) {
      |                                ^
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h:289:23: note: ‘constexpr const uint8_t ARRAY_INITIAL_VALS [50]’ declared here
  289 |     constexpr uint8_t ARRAY_INITIAL_VALS[] = {
      |                       ^~~~~~~~~~~~~~~~~~
[1106/1114] Building CXX object libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.quick_sort_test.__unit__.__build__.dir/quick_sort_test.cpp.o
FAILED: libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.quick_sort_test.__unit__.__build__.dir/quick_sort_test.cpp.o 
/usr/bin/g++ -DLIBC_NAMESPACE=__llvm_libc_20_0_0_git -D_DEBUG -I/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc -isystem /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/build/libc/include -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wimplicit-fallthrough -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -fpie -DLIBC_FULL_BUILD -ffreestanding -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -fext-numeric-literals -Wno-pedantic -std=gnu++17 -MD -MT libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.quick_sort_test.__unit__.__build__.dir/quick_sort_test.cpp.o -MF libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.quick_sort_test.__unit__.__build__.dir/quick_sort_test.cpp.o.d -o libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.quick_sort_test.__unit__.__build__.dir/quick_sort_test.cpp.o -c /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/quick_sort_test.cpp
In file included from /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/quick_sort_test.cpp:9:
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h: In lambda function:
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h:310:34: error: ‘ARRAY_INITIAL_VALS’ is not captured
  310 |         const uint8_t elem_val = ARRAY_INITIAL_VALS[elem_i];
      |                                  ^~~~~~~~~~~~~~~~~~
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h:304:32: note: the lambda has no capture-default
  304 |     const auto fill_buf = [&buf](size_t elem_size) {
      |                                ^
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/llvm-project/libc/test/src/stdlib/SortingTest.h:289:23: note: ‘constexpr const uint8_t ARRAY_INITIAL_VALS [50]’ declared here
  289 |     constexpr uint8_t ARRAY_INITIAL_VALS[] = {
      |                       ^~~~~~~~~~~~~~~~~~
[1107/1114] Building CXX object libc/test/src/stdlib/CMakeFiles/libc.test.src.stdlib.qsort_r_test.__unit__.__build__.dir/qsort_r_test.cpp.o
[1108/1114] Running unit test libc.test.src.time.nanosleep_test.__unit__
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcNanosleep.SmokeTest
[       OK ] LlvmLibcNanosleep.SmokeTest (32 us)
Ran 1 tests.  PASS: 1  FAIL: 0
ninja: build stopped: subcommand failed.
['ninja', 'libc-unit-tests'] exited with return code 1.
The build step threw an exception...
Traceback (most recent call last):
  File "/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-gcc-fullbuild-dbg/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 164, in step
    yield

@SchrodingerZhu
Copy link
Contributor

New build problem should be an easy fix.

@SchrodingerZhu
Copy link
Contributor

#121684

JoelWee added a commit to JoelWee/llvm-project that referenced this pull request Jan 6, 2025
JoelWee added a commit that referenced this pull request Jan 6, 2025
JoelWee added a commit that referenced this pull request Jan 6, 2025
paulhuggett pushed a commit to paulhuggett/llvm-project that referenced this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants