Merge pull request 'Replace SIMD assembly with initialized Node16::index + valgrind client request' (#77) from client-request-instead-of-asm into main

Reviewed-on: #77

.gitea/workflows/ci.yml

@@ -38,21 +38,43 @@ jobs:
       matrix:
         include:
           - name: 64-bit-versions
+            runner: ubuntu-latest-amd64
+            arch: amd64
+            llvm_version: "21"
+            msan_url: ""
             cmake_args: -DCMAKE_CXX_FLAGS=-DUSE_64_BIT=1
           - name: debug
+            runner: ubuntu-latest-amd64
+            arch: amd64
+            llvm_version: "21"
+            msan_url: https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst
             cmake_args: -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan
           - name: simd-fallback
+            runner: ubuntu-latest-amd64
+            arch: amd64
+            llvm_version: "21"
+            msan_url: ""
             cmake_args: -DUSE_SIMD_FALLBACK=ON
           - name: gcc
+            runner: ubuntu-latest-amd64
+            arch: amd64
+            llvm_version: "21"
+            msan_url: ""
             cmake_args: -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++
-    runs-on: ubuntu-latest-amd64
+          - name: debug-arm64
+            runner: ubuntu-latest-arm64
+            arch: arm64
+            llvm_version: "22"
+            msan_url: https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst
+            cmake_args: -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan
+    runs-on: ${{ matrix.runner }}
     steps:
       - uses: actions/checkout@v4
 
       - uses: actions/cache@v4
         with:
           path: /var/cache/apt/archives
-          key: apt-amd64-${{ hashFiles('.gitea/workflows/ci.yml') }}
+          key: apt-${{ matrix.arch }}-${{ hashFiles('.gitea/workflows/ci.yml') }}
 
       - name: Install common dependencies
         run: |
@@ -68,18 +90,18 @@ jobs:
         run: |
           . /etc/os-release
           wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key | sudo tee /etc/apt/trusted.gpg.d/apt.llvm.org.asc
-          echo "deb http://apt.llvm.org/${VERSION_CODENAME}/ llvm-toolchain-${VERSION_CODENAME}-21 main" | sudo tee /etc/apt/sources.list.d/llvm.list
+          echo "deb http://apt.llvm.org/${VERSION_CODENAME}/ llvm-toolchain-${VERSION_CODENAME}-${{ matrix.llvm_version }} main" | sudo tee /etc/apt/sources.list.d/llvm.list
           sudo apt-get update -qq
           sudo apt-get install -y \
-            clang-21 llvm-21 lld-21 mold
+            clang-${{ matrix.llvm_version }} llvm-${{ matrix.llvm_version }} lld-${{ matrix.llvm_version }} mold
           for tool in clang clang++ llvm-ar llvm-nm llvm-ranlib llvm-objcopy llvm-cov llvm-symbolizer lld ld.lld; do
-            sudo update-alternatives --install /usr/bin/${tool} ${tool} /usr/bin/${tool}-21 100
+            sudo update-alternatives --install /usr/bin/${tool} ${tool} /usr/bin/${tool}-${{ matrix.llvm_version }} 100
           done
 
       - name: Download MSan toolchain
-        if: matrix.name == 'debug'
+        if: matrix.msan_url != ''
         run: |
-          curl -Ls "https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst" -o /tmp/msan-toolchain.tar.zst
+          curl -Ls "${{ matrix.msan_url }}" -o /tmp/msan-toolchain.tar.zst
           sudo mkdir -p /opt/msan
           sudo tar --zstd -xf /tmp/msan-toolchain.tar.zst -C /opt/msan
 
@@ -185,12 +207,7 @@ jobs:
       - name: Test
         run: |
           cd build
-          # On arm64, valgrind needs the MAKE_MEM_DEFINED client requests for
+          ctest --no-compress-output --test-output-size-passed 100000 --test-output-size-failed 100000 -T Test -j "$(nproc)" --timeout 90 > /dev/null
-          # https://git.weaselab.dev/weaselab/conflict-set/issues/39, but this
-          # build has -DNVALGRIND, which compiles them out. Skip valgrind
-          # tests here; they run annotated in the test job.
-          ctest --no-compress-output --test-output-size-passed 100000 --test-output-size-failed 100000 ${{ matrix.arch == 'arm64' && '-E valgrind' || '' }} -T Test -j "$(nproc)" --timeout 90 > /dev/null
-
       - name: Package
         run: |
           cd build

CMakeLists.txt

@@ -5,7 +5,7 @@ project(
   DESCRIPTION
     "A data structure for optimistic concurrency control on ranges of bitwise-lexicographically-ordered keys."
   HOMEPAGE_URL "https://git.weaselab.dev/weaselab/conflict-set"
-  LANGUAGES C CXX ASM)
+  LANGUAGES C CXX)
 set(CMAKE_CXX_STANDARD 20)
 
 file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/version.txt ${PROJECT_VERSION})
@@ -130,15 +130,7 @@ endif()
 
 set(CMAKE_CXX_IMPLICIT_LINK_LIBRARIES "")
 
-# Architecture-specific SIMD assembly. These functions operate on
+add_library(${PROJECT_NAME}-object OBJECT ConflictSet.cpp)
-# potentially-indeterminate memory, which is UB in C++ but well-defined in
-# assembly.
-set(SIMD_ASM_FILES)
-if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 AND NOT USE_SIMD_FALLBACK)
-  set(SIMD_ASM_FILES ${CMAKE_CURRENT_SOURCE_DIR}/simd_x86_64.S)
-endif()
-
-add_library(${PROJECT_NAME}-object OBJECT ConflictSet.cpp ${SIMD_ASM_FILES})
 target_compile_options(${PROJECT_NAME}-object PRIVATE -fno-exceptions
                                                       -fvisibility=hidden)
 target_include_directories(${PROJECT_NAME}-object
@@ -241,7 +233,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
   endif()
 
   # ad hoc testing
-  add_executable(conflict_set_main ConflictSet.cpp ${SIMD_ASM_FILES})
+  add_executable(conflict_set_main ConflictSet.cpp)
   target_include_directories(conflict_set_main
                              PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
   target_compile_definitions(conflict_set_main PRIVATE ENABLE_MAIN)
@@ -257,7 +249,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
     cmake_pop_check_state()
 
     if(HAS_LIB_FUZZER)
-      add_executable(conflict_set_fuzz_test ConflictSet.cpp ${SIMD_ASM_FILES})
+      add_executable(conflict_set_fuzz_test ConflictSet.cpp)
       target_include_directories(conflict_set_fuzz_test
                                  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
       target_compile_definitions(conflict_set_fuzz_test PRIVATE ENABLE_FUZZ)
@@ -269,8 +261,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
   endif()
 
   # whitebox tests asan+ubsan
-  add_executable(fuzz_driver ConflictSet.cpp FuzzTestDriver.cpp
+  add_executable(fuzz_driver ConflictSet.cpp FuzzTestDriver.cpp)
-                             ${SIMD_ASM_FILES})
   target_compile_options(fuzz_driver PRIVATE ${TEST_FLAGS})
   if(NOT CMAKE_CROSSCOMPILING)
     target_compile_options(fuzz_driver PRIVATE -fsanitize=address,undefined)
@@ -286,8 +277,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
 
   # whitebox tests msan
   if(MSAN_TOOLCHAIN_PATH)
-    add_executable(fuzz_driver_msan ConflictSet.cpp FuzzTestDriver.cpp
+    add_executable(fuzz_driver_msan ConflictSet.cpp FuzzTestDriver.cpp)
-                                    ${SIMD_ASM_FILES})
     target_compile_options(fuzz_driver_msan PRIVATE ${TEST_FLAGS})
     if(NOT CMAKE_CROSSCOMPILING)
       target_compile_options(
@@ -315,8 +305,7 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
 
   # tsan tests
   if(NOT CMAKE_CROSSCOMPILING AND NOT DISABLE_TSAN)
-    add_executable(tsan_driver ConflictSet.cpp FuzzTestDriver.cpp
+    add_executable(tsan_driver ConflictSet.cpp FuzzTestDriver.cpp)
-                               ${SIMD_ASM_FILES})
     target_compile_options(tsan_driver PRIVATE ${TEST_FLAGS} -fsanitize=thread)
     target_link_options(tsan_driver PRIVATE -fsanitize=thread)
     target_compile_definitions(tsan_driver PRIVATE ENABLE_FUZZ THREAD_TEST)

ConflictSet.cpp

@@ -28,7 +28,6 @@ limitations under the License.
 #include "Internal.h"
 #include "LongestCommonPrefix.h"
 #include "Metrics.h"
-#include "simd.h"
 
 #include <algorithm>
 #include <bit>
@@ -489,6 +488,7 @@ inline void Node3::copyChildrenAndKeyFrom(const Node16 &other) {
 
 inline void Node16::copyChildrenAndKeyFrom(const Node3 &other) {
   copyCommon(*this, other);
+  memset(index, 0, sizeof(index));
   memcpy(index, other.index, Node3::kMaxNodes);
   memcpy(children, other.children,
          Node3::kMaxNodes * sizeof(children[0])); // NOLINT
@@ -500,6 +500,12 @@ inline void Node16::copyChildrenAndKeyFrom(const Node3 &other) {
     assert(children[i]->parent == &other);
     children[i]->parent = this;
   }
+  // The index bytes beyond numChildren are initialized so that the SIMD loads
+  // of the full 16 bytes are well-defined (reading indeterminate bytes is UB
+  // in C++). valgrind then treats them as undefined so that it can still flag
+  // any read that relies on them.
+  VALGRIND_MAKE_MEM_UNDEFINED(index + numChildren,
+                              sizeof(index) - numChildren * sizeof(index[0]));
 }
 
 inline void Node16::copyChildrenAndKeyFrom(const Node16 &other) {
@@ -516,6 +522,7 @@ inline void Node16::copyChildrenAndKeyFrom(const Node16 &other) {
 
 inline void Node16::copyChildrenAndKeyFrom(const Node48 &other) {
   copyCommon(*this, other);
+  memset(index, 0, sizeof(index));
   int i = 0;
   other.bitSet.forEachSet([&](int c) {
     // Suppress a false positive -Waggressive-loop-optimizations warning
@@ -529,6 +536,12 @@ inline void Node16::copyChildrenAndKeyFrom(const Node48 &other) {
     ++i;
   });
   memcpy(partialKey(), &other + 1, partialKeyLen);
+  // The index bytes beyond numChildren are initialized so that the SIMD loads
+  // of the full 16 bytes are well-defined (reading indeterminate bytes is UB
+  // in C++). valgrind then treats them as undefined so that it can still flag
+  // any read that relies on them.
+  VALGRIND_MAKE_MEM_UNDEFINED(index + numChildren,
+                              sizeof(index) - numChildren * sizeof(index[0]));
 }
 
 inline void Node48::copyChildrenAndKeyFrom(const Node16 &other) {
@@ -536,6 +549,7 @@ inline void Node48::copyChildrenAndKeyFrom(const Node16 &other) {
   assert(numChildren == Node16::kMaxNodes);
   memset(index, -1, sizeof(index));
   memset(children, 0, sizeof(children));
+  memset(reverseIndex, 0, sizeof(reverseIndex));
   const auto z = InternalVersionT::zero;
   for (auto &v : childMaxVersion) {
     v = z;
@@ -555,6 +569,13 @@ inline void Node48::copyChildrenAndKeyFrom(const Node16 &other) {
         std::max(maxOfMax[i >> Node48::kMaxOfMaxShift], childMaxVersion[i]);
     ++i;
   }
+  // The reverseIndex bytes beyond numChildren are initialized so that the
+  // SIMD loads of a full 16-byte page in scan16 are well-defined (reading
+  // indeterminate bytes is UB in C++). valgrind then treats them as undefined
+  // so that it can still flag any read that relies on them.
+  VALGRIND_MAKE_MEM_UNDEFINED(reverseIndex + numChildren,
+                              sizeof(reverseIndex) -
+                                  numChildren * sizeof(reverseIndex[0]));
 }
 
 inline void Node48::copyChildrenAndKeyFrom(const Node48 &other) {
@@ -581,6 +602,7 @@ inline void Node48::copyChildrenAndKeyFrom(const Node256 &other) {
   copyCommon(*this, other);
   memset(index, -1, sizeof(index));
   memset(children, 0, sizeof(children));
+  memset(reverseIndex, 0, sizeof(reverseIndex));
   const auto z = InternalVersionT::zero;
   for (auto &v : childMaxVersion) {
     v = z;
@@ -602,6 +624,13 @@ inline void Node48::copyChildrenAndKeyFrom(const Node256 &other) {
     ++i;
   });
   memcpy(partialKey(), &other + 1, partialKeyLen);
+  // The reverseIndex bytes beyond numChildren are initialized so that the
+  // SIMD loads of a full 16-byte page in scan16 are well-defined (reading
+  // indeterminate bytes is UB in C++). valgrind then treats them as undefined
+  // so that it can still flag any read that relies on them.
+  VALGRIND_MAKE_MEM_UNDEFINED(reverseIndex + numChildren,
+                              sizeof(reverseIndex) -
+                                  numChildren * sizeof(reverseIndex[0]));
 }
 
 inline void Node256::copyChildrenAndKeyFrom(const Node48 &other) {
@@ -911,11 +940,34 @@ int getNodeIndexExists(Node3 *self, uint8_t index) {
 
 int getNodeIndex(Node16 *self, uint8_t index) {
 
-#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
+#ifdef HAS_AVX
-  uint32_t bitfield =
+  // Based on https://www.the-paper-trail.org/post/art-paper-notes/
-      find_eq_16(self->index, index) & ((1 << self->numChildren) - 1);
+
+  // key_vec is 16 repeated copies of the searched-for byte, one for every
+  // possible position in child_keys that needs to be searched.
+  __m128i key_vec = _mm_set1_epi8(index);
+
+  // Compare all child_keys to 'index' in parallel. Don't worry if some of the
+  // keys aren't valid, we'll mask the results to only consider the valid ones
+  // below.
+  __m128i indices;
+  memcpy(&indices, self->index, Node16::kMaxNodes);
+  __m128i results = _mm_cmpeq_epi8(key_vec, indices);
+
+  // Build a mask to select only the first node->num_children values from the
+  // comparison (because the other values are meaningless)
+  uint32_t mask = (1 << self->numChildren) - 1;
+
+  // Change the results of the comparison into a bitfield, masking off any
+  // invalid comparisons.
+  uint32_t bitfield = _mm_movemask_epi8(results) & mask;
+
+  // No match if there are no '1's in the bitfield.
   if (bitfield == 0)
     return -1;
+
+  // Find the index of the first '1' in the bitfield by counting the leading
+  // zeros.
   return std::countr_zero(bitfield);
 #elif defined(HAS_ARM_NEON)
   // Based on
@@ -948,9 +1000,13 @@ int getNodeIndex(Node16 *self, uint8_t index) {
 
 int getNodeIndexExists(Node16 *self, uint8_t index) {
 
-#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
+#ifdef HAS_AVX
-  uint32_t bitfield =
+  __m128i key_vec = _mm_set1_epi8(index);
-      find_eq_16(self->index, index) & ((1 << self->numChildren) - 1);
+  __m128i indices;
+  memcpy(&indices, self->index, Node16::kMaxNodes);
+  __m128i results = _mm_cmpeq_epi8(key_vec, indices);
+  uint32_t mask = (1 << self->numChildren) - 1;
+  uint32_t bitfield = _mm_movemask_epi8(results) & mask;
   assume(bitfield != 0);
   return std::countr_zero(bitfield);
 #elif defined(HAS_ARM_NEON)
@@ -1240,9 +1296,13 @@ TaggedNodePointer getChildGeq(Node16 *self, int child) {
     return nullptr;
   }
 
-#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
+#ifdef HAS_AVX
-  uint32_t bitfield =
+  __m128i key_vec = _mm_set1_epi8(child);
-      find_ge_16(self->index, child) & ((1 << self->numChildren) - 1);
+  __m128i indices;
+  memcpy(&indices, self->index, Node16::kMaxNodes);
+  __m128i results = _mm_cmpeq_epi8(key_vec, _mm_min_epu8(key_vec, indices));
+  int mask = (1 << self->numChildren) - 1;
+  uint32_t bitfield = _mm_movemask_epi8(results) & mask;
   return bitfield == 0 ? nullptr : self->children[std::countr_zero(bitfield)];
 #elif defined(HAS_ARM_NEON)
   uint8x16_t indices;
@@ -1965,6 +2025,8 @@ Node *erase(Node *self, WriteContext *writeContext, bool logical) {
     parent48->childMaxVersion[lastChildrenIndex] = writeContext->zero;
     VALGRIND_MAKE_MEM_UNDEFINED(parent48->children + lastChildrenIndex,
                                 sizeof(parent48->children[0]));
+    VALGRIND_MAKE_MEM_UNDEFINED(parent48->reverseIndex + lastChildrenIndex,
+                                sizeof(parent48->reverseIndex[0]));
 
     if (needsDownsize(parent48)) {
       downsize(parent48, writeContext);
@@ -2057,6 +2119,52 @@ compare16_avx512(const InternalVersionT *vs, InternalVersionT rv) {
 }
 #endif
 
+#ifdef HAS_ARM_NEON
+// Returns a uint8x16_t whose byte i is 0xff if vs[i] > readVersion and 0
+// otherwise. When USE_64_BIT is set the comparison uses the full 64-bit value
+// (via vcgtq_s64); otherwise it falls back to the 32-bit subtract-and-compare
+// used by the original NEON fast path. The caller narrows this 16-byte vector
+// into the nibble-packed (4 bits per version) mask expected by scan16 and
+// checkMaxBetweenExclusiveImpl.
+inline uint8x16_t conflictMask16(const InternalVersionT *vs,
+                                 InternalVersionT readVersion) {
+#if USE_64_BIT
+  int64_t rv;
+  memcpy(&rv, &readVersion, sizeof(rv));
+  const auto rvVec = vdupq_n_s64(rv);
+  int32x2_t r32[8];
+  const auto *vsp = reinterpret_cast<const int64_t *>(vs);
+  for (int j = 0; j < 8; ++j) {
+    r32[j] = vmovn_s64(vcgtq_s64(vld1q_s64(vsp + 2 * j), rvVec));
+  }
+  uint32x4_t w4[4];
+  for (int k = 0; k < 4; ++k) {
+    w4[k] = vreinterpretq_u32_s32(vcombine_s32(r32[2 * k], r32[2 * k + 1]));
+  }
+  return vcombine_u8(
+      vmovn_u16(vcombine_u16(vmovn_u32(w4[0]), vmovn_u32(w4[1]))),
+      vmovn_u16(vcombine_u16(vmovn_u32(w4[2]), vmovn_u32(w4[3]))));
+#else
+  uint32x4_t w4[4];
+  memcpy(w4, vs, sizeof(w4));
+  uint32_t rv;
+  memcpy(&rv, &readVersion, sizeof(rv));
+  const auto rvVec = vdupq_n_u32(rv);
+
+  int32x4_t z;
+  memset(&z, 0, sizeof(z));
+
+  uint16x4_t conflicting[4];
+  for (int i = 0; i < 4; ++i) {
+    conflicting[i] =
+        vmovn_u32(vcgtq_s32(vreinterpretq_s32_u32(vsubq_u32(w4[i], rvVec)), z));
+  }
+  return vcombine_u8(vmovn_u16(vcombine_u16(conflicting[0], conflicting[1])),
+                     vmovn_u16(vcombine_u16(conflicting[2], conflicting[3])));
+#endif
+}
+#endif
+
 // Returns true if v[i] <= readVersion for all i such that begin <= is[i] < end
 // Preconditions: begin <= end, end - begin < 256
 template <bool kAVX512>
@@ -2077,32 +2185,20 @@ bool scan16(const InternalVersionT *vs, const uint8_t *is, int begin, int end,
   uint64_t mask = vget_lane_u64(
       vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(results), 4)), 0);
 
-  uint32x4_t w4[4];
-  memcpy(w4, vs, sizeof(w4));
-  uint32_t rv;
-  memcpy(&rv, &readVersion, sizeof(rv));
-  const auto rvVec = vdupq_n_u32(rv);
-
-  int32x4_t z;
-  memset(&z, 0, sizeof(z));
-
-  uint16x4_t conflicting[4];
-  for (int i = 0; i < 4; ++i) {
-    conflicting[i] =
-        vmovn_u32(vcgtq_s32(vreinterpretq_s32_u32(vsubq_u32(w4[i], rvVec)), z));
-  }
-  auto combined =
-      vcombine_u8(vmovn_u16(vcombine_u16(conflicting[0], conflicting[1])),
-                  vmovn_u16(vcombine_u16(conflicting[2], conflicting[3])));
-
   uint64_t compared = vget_lane_u64(
-      vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(combined), 4)), 0);
+      vreinterpret_u64_u8(vshrn_n_u16(
+          vreinterpretq_u16_u8(conflictMask16(vs, readVersion)), 4)),
+      0);
 
   return !(compared & mask);
 
-#elif defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
+#elif defined(HAS_AVX)
 
-  uint32_t mask = mask_in_range_16(is, begin, end);
+  __m128i indices;
+  memcpy(&indices, is, 16);
+  indices = _mm_sub_epi8(indices, _mm_set1_epi8(begin));
+  uint32_t mask = ~_mm_movemask_epi8(_mm_cmpeq_epi8(
+      indices, _mm_max_epu8(indices, _mm_set1_epi8(end - begin))));
 
   uint32_t compared = 0;
   if constexpr (kAVX512) {
@@ -2142,26 +2238,10 @@ bool scan16(const InternalVersionT *vs, int begin, int end,
   assert(begin <= end);
 
 #if defined(HAS_ARM_NEON)
-  uint32x4_t w4[4];
-  memcpy(w4, vs, sizeof(w4));
-  uint32_t rv;
-  memcpy(&rv, &readVersion, sizeof(rv));
-  const auto rvVec = vdupq_n_u32(rv);
-
-  int32x4_t z;
-  memset(&z, 0, sizeof(z));
-
-  uint16x4_t conflicting[4];
-  for (int i = 0; i < 4; ++i) {
-    conflicting[i] =
-        vmovn_u32(vcgtq_s32(vreinterpretq_s32_u32(vsubq_u32(w4[i], rvVec)), z));
-  }
-  auto combined =
-      vcombine_u8(vmovn_u16(vcombine_u16(conflicting[0], conflicting[1])),
-                  vmovn_u16(vcombine_u16(conflicting[2], conflicting[3])));
-
   uint64_t conflict = vget_lane_u64(
-      vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(combined), 4)), 0);
+      vreinterpret_u64_u8(vshrn_n_u16(
+          vreinterpretq_u16_u8(conflictMask16(vs, readVersion)), 4)),
+      0);
 
   conflict &= end == 16 ? -1 : (uint64_t(1) << (end << 2)) - 1;
   conflict >>= begin << 2;
@@ -2275,33 +2355,23 @@ bool checkMaxBetweenExclusiveImpl(Node16 *n, int begin, int end,
   const bool firstRangeOk =
       !child->entryPresent || child->entry.rangeVersion <= readVersion;
 
-  uint32x4_t w4[4];
-  memcpy(w4, self->childMaxVersion, sizeof(w4));
-  uint32_t rv;
-  memcpy(&rv, &readVersion, sizeof(rv));
-  const auto rvVec = vdupq_n_u32(rv);
-
-  int32x4_t z;
-  memset(&z, 0, sizeof(z));
-
-  uint16x4_t conflicting[4];
-  for (int i = 0; i < 4; ++i) {
-    conflicting[i] =
-        vmovn_u32(vcgtq_s32(vreinterpretq_s32_u32(vsubq_u32(w4[i], rvVec)), z));
-  }
-  auto combined =
-      vcombine_u8(vmovn_u16(vcombine_u16(conflicting[0], conflicting[1])),
-                  vmovn_u16(vcombine_u16(conflicting[2], conflicting[3])));
-
   uint64_t compared = vget_lane_u64(
-      vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(combined), 4)), 0);
+      vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(conflictMask16(
+                                          self->childMaxVersion, readVersion)),
+                                      4)),
+      0);
 
   return !(compared & mask) && firstRangeOk;
 
-#elif defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
+#elif defined(HAS_AVX)
 
-  uint32_t mask = mask_in_range_16(self->index, begin, end) &
+  __m128i indices;
-                  ((1 << self->numChildren) - 1);
+  memcpy(&indices, self->index, 16);
+  indices = _mm_sub_epi8(indices, _mm_set1_epi8(begin));
+  uint32_t mask =
+      0xffff & ~_mm_movemask_epi8(_mm_cmpeq_epi8(
+                   indices, _mm_max_epu8(indices, _mm_set1_epi8(end - begin))));
+  mask &= (1 << self->numChildren) - 1;
   if (!mask) {
     return true;
   }

simd.h

@@ -1,25 +0,0 @@
-#pragma once
-
-#include <cstdint>
-
-#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
-
-// SIMD operations on potentially-indeterminate Node16::index[16] bytes.
-// Implemented in file-level assembly (simd_x86_64.S) because loading and
-// operating on indeterminate values is UB in C++ but well-defined in
-// assembly. The caller must mask the returned bitfield to
-// [0, numChildren) before using it.
-//
-// Each function returns a 16-bit bitmask in the low 16 bits of a uint32_t
-// (upper 16 bits are zero). Bit i is set iff the condition holds at index i.
-
-extern "C" {
-// Returns bit i set iff idx[i] == key
-uint32_t find_eq_16(const uint8_t idx[16], uint8_t key);
-// Returns bit i set iff idx[i] >= child
-uint32_t find_ge_16(const uint8_t idx[16], uint8_t child);
-// Returns bit i set iff begin <= idx[i] < end
-uint32_t mask_in_range_16(const uint8_t idx[16], uint8_t begin, uint8_t end);
-}
-
-#endif

simd_x86_64.S

@@ -1,86 +0,0 @@
-// SIMD operations on potentially-indeterminate Node16::index[16] bytes.
-// Written in assembly because loading and operating on indeterminate values
-// is UB in C++ but well-defined in assembly. (A side effect is that msan
-// doesn't track the loads.) The caller is responsible for masking the
-// returned bitfield to [0, numChildren) before using it.
-//
-// All functions return a 16-bit bitmask in %eax (bit i set = condition true
-// at index i). The upper 16 bits of %eax are zero.
-//
-// System V AMD64 ABI:
-//   %rdi = const uint8_t *idx  (16 bytes)
-//   %esi = uint8_t key         (find_eq_16, find_ge_16)
-//   %sil = uint8_t begin       (mask_in_range_16)
-//   %dl  = uint8_t end         (mask_in_range_16)
-
-    .text
-
-// uint32_t find_eq_16(const uint8_t idx[16], uint8_t key)
-//   Returns bit i set if idx[i] == key
-    .globl find_eq_16
-    .type  find_eq_16, @function
-find_eq_16:
-    vmovd      %esi, %xmm1          // broadcast key
-    vpbroadcastb %xmm1, %xmm1
-    vmovdqu    (%rdi), %xmm0        // load 16 bytes (may contain indeterminate data)
-    vpcmpeqb   %xmm0, %xmm1, %xmm0  // 0xff for each match
-    vpmovmskb  %xmm0, %eax          // 16-bit bitmask
-    movzwl     %ax, %eax            // zero-extend to 32 bits
-    ret
-    .size find_eq_16, .-find_eq_16
-
-// uint32_t find_ge_16(const uint8_t idx[16], uint8_t child)
-//   Returns bit i set if idx[i] >= child
-//   x86 doesn't have a "compare unsigned >=" for bytes directly, so we use:
-//   min(key, idx[i]) == key  iff  idx[i] >= key
-    .globl find_ge_16
-    .type  find_ge_16, @function
-find_ge_16:
-    vmovd      %esi, %xmm1
-    vpbroadcastb %xmm1, %xmm1       // key broadcast
-    vmovdqu    (%rdi), %xmm0        // load 16 bytes
-    vpminub    %xmm0, %xmm1, %xmm2  // min(key, idx[i])
-    vpcmpeqb   %xmm2, %xmm1, %xmm0  // 0xff where min == key, i.e. idx[i] >= key
-    vpmovmskb  %xmm0, %eax
-    movzwl     %ax, %eax
-    ret
-    .size find_ge_16, .-find_ge_16
-
-// uint32_t mask_in_range_16(const uint8_t idx[16], uint8_t begin, uint8_t end)
-//   Returns bit i set if begin <= idx[i] < end
-//   Logic: (idx[i] - begin) < (end - begin)  [unsigned wrapping arithmetic]
-//   Equivalently: idx[i] - begin != max(idx[i] - begin, end - begin)
-//   i.e. idx[i] - begin is NOT equal to the saturated value.
-//   We compute: sub = idx - begin; result = (sub < (end-begin)) for each byte.
-//   Using: sub == max(sub, end-begin) means NOT in range.
-//   So: in_range = ~(movemask(cmpeq(sub, max(sub, range_size))))
-    .globl mask_in_range_16
-    .type  mask_in_range_16, @function
-mask_in_range_16:
-    vmovd      %esi, %xmm1          // begin
-    vpbroadcastb %xmm1, %xmm1
-    vmovd      %edx, %xmm2          // end
-    vpbroadcastb %xmm2, %xmm2
-    vmovdqu    (%rdi), %xmm0        // load 16 bytes
-    vpsubb     %xmm1, %xmm0, %xmm0  // idx - begin (wrapping)
-    vpsubb     %xmm1, %xmm2, %xmm2  // end - begin (range size)
-    vpmaxub    %xmm0, %xmm2, %xmm3  // max(idx-begin, range_size)
-    vpcmpeqb   %xmm3, %xmm0, %xmm0  // 0xff where NOT in range
-    vpmovmskb  %xmm0, %eax
-    not        %eax                 // invert: 1 = in range
-    movzwl     %ax, %eax
-    ret
-    .size mask_in_range_16, .-mask_in_range_16
-
-    .section .note.gnu.property,"a",@note
-    .p2align 3, 0x0
-    .long 4
-    .long 16
-    .long 5
-    .asciz "GNU"
-    .long 0xc0000002
-    .long 4
-    .long 0x3
-    .p2align 3, 0x0
-
-    .section .note.GNU-stack,"",@progbits