From bae2b22e95d8b19b9c78fdf7dde9e9ea34cd1e69 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Mon, 3 Aug 2026 17:19:43 -0400 Subject: [PATCH 1/2] Replace SIMD assembly with initialized Node16::index + valgrind client request Commit 6fed133 moved SIMD operations on Node16::index into file-level assembly (later extended to aarch64) to avoid reading indeterminate bytes (UB in C++). Revert that in favor of a simpler, architecture-independent fix: - Restore the original HAS_AVX / HAS_ARM_NEON intrinsics at the affected call sites, keeping the newer conflictMask16 helper and scalar fallback. - memset Node16::index in both copy constructors (from Node3 and Node48) so the [numChildren, 16) tail is always initialized, making the full 16-byte SIMD loads well-defined. This also fixes the same latent UB in the NEON path and the Node16->Node16 full-array memcpy. - Issue a VALGRIND_MAKE_MEM_UNDEFINED client request on the tail so valgrind still flags any read that relies on the unused slots rather than numChildren. - Drop simd.h, simd_x86_64.S, simd_aarch64.S, and the CMake ASM wiring. --- CMakeLists.txt | 29 ++------- ConflictSet.cpp | 169 +++++++++++++++++++++++++++++++++++++----------- simd.h | 48 -------------- simd_aarch64.S | 84 ------------------------ simd_x86_64.S | 86 ------------------------ 5 files changed, 139 insertions(+), 277 deletions(-) delete mode 100644 simd.h delete mode 100644 simd_aarch64.S delete mode 100644 simd_x86_64.S diff --git a/CMakeLists.txt b/CMakeLists.txt index a265e5d..733818c 100644 --- a/CMakeLists.txt +++ b/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,19 +130,7 @@ endif() set(CMAKE_CXX_IMPLICIT_LINK_LIBRARIES "") -# Architecture-specific SIMD assembly. These functions operate on -# 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) -elseif((CMAKE_SYSTEM_PROCESSOR STREQUAL aarch64 OR CMAKE_SYSTEM_PROCESSOR - STREQUAL arm64) - AND NOT USE_SIMD_FALLBACK) - set(SIMD_ASM_FILES ${CMAKE_CURRENT_SOURCE_DIR}/simd_aarch64.S) -endif() - -add_library(${PROJECT_NAME}-object OBJECT ConflictSet.cpp ${SIMD_ASM_FILES}) +add_library(${PROJECT_NAME}-object OBJECT ConflictSet.cpp) target_compile_options(${PROJECT_NAME}-object PRIVATE -fno-exceptions -fvisibility=hidden) target_include_directories(${PROJECT_NAME}-object @@ -245,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) @@ -261,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) @@ -273,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 - ${SIMD_ASM_FILES}) + add_executable(fuzz_driver ConflictSet.cpp FuzzTestDriver.cpp) target_compile_options(fuzz_driver PRIVATE ${TEST_FLAGS}) if(NOT CMAKE_CROSSCOMPILING) target_compile_options(fuzz_driver PRIVATE -fsanitize=address,undefined) @@ -290,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 - ${SIMD_ASM_FILES}) + add_executable(fuzz_driver_msan ConflictSet.cpp FuzzTestDriver.cpp) target_compile_options(fuzz_driver_msan PRIVATE ${TEST_FLAGS}) if(NOT CMAKE_CROSSCOMPILING) target_compile_options( @@ -319,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 - ${SIMD_ASM_FILES}) + add_executable(tsan_driver ConflictSet.cpp FuzzTestDriver.cpp) 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) diff --git a/ConflictSet.cpp b/ConflictSet.cpp index af90c86..a82d0d1 100644 --- a/ConflictSet.cpp +++ b/ConflictSet.cpp @@ -28,7 +28,6 @@ limitations under the License. #include "Internal.h" #include "LongestCommonPrefix.h" #include "Metrics.h" -#include "simd.h" #include #include @@ -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,21 +940,51 @@ int getNodeIndexExists(Node3 *self, uint8_t index) { int getNodeIndex(Node16 *self, uint8_t index) { -#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK) - uint32_t bitfield = - find_eq_16(self->index, index) & ((1 << self->numChildren) - 1); +#ifdef HAS_AVX + // Based on https://www.the-paper-trail.org/post/art-paper-notes/ + + // 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) - // The index load is done in assembly (find_eq_16) so that reading the - // potentially-indeterminate unused index bytes is well-defined. - uint64_t bitfield = find_eq_16(self->index, index); + // Based on + // https://community.arm.com/arm-community-blogs/b/infrastructure-solutions-blog/posts/porting-x86-vector-bitmask-optimizations-to-arm-neon + + uint8x16_t indices; + memcpy(&indices, self->index, Node16::kMaxNodes); + // 0xff for each match + uint16x8_t results = + vreinterpretq_u16_u8(vceqq_u8(vdupq_n_u8(index), indices)); assume(self->numChildren <= Node16::kMaxNodes); uint64_t mask = self->numChildren == 16 ? uint64_t(-1) : (uint64_t(1) << (self->numChildren * 4)) - 1; - bitfield &= mask; + // 0xf for each match in valid range + uint64_t bitfield = + vget_lane_u64(vreinterpret_u64_u8(vshrn_n_u16(results, 4)), 0) & mask; if (bitfield == 0) return -1; return std::countr_zero(bitfield) / 4; @@ -941,20 +1000,31 @@ int getNodeIndex(Node16 *self, uint8_t index) { int getNodeIndexExists(Node16 *self, uint8_t index) { -#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK) - uint32_t bitfield = - find_eq_16(self->index, index) & ((1 << self->numChildren) - 1); +#ifdef HAS_AVX + __m128i key_vec = _mm_set1_epi8(index); + __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) - // The index load is done in assembly (find_eq_16) so that reading the - // potentially-indeterminate unused index bytes is well-defined. - uint64_t bitfield = find_eq_16(self->index, index); + // Based on + // https://community.arm.com/arm-community-blogs/b/infrastructure-solutions-blog/posts/porting-x86-vector-bitmask-optimizations-to-arm-neon + + uint8x16_t indices; + memcpy(&indices, self->index, Node16::kMaxNodes); + // 0xff for each match + uint16x8_t results = + vreinterpretq_u16_u8(vceqq_u8(vdupq_n_u8(index), indices)); assume(self->numChildren <= Node16::kMaxNodes); uint64_t mask = self->numChildren == 16 ? uint64_t(-1) : (uint64_t(1) << (self->numChildren * 4)) - 1; - bitfield &= mask; + // 0xf for each match in valid range + uint64_t bitfield = + vget_lane_u64(vreinterpret_u64_u8(vshrn_n_u16(results, 4)), 0) & mask; assume(bitfield != 0); return std::countr_zero(bitfield) / 4; #else @@ -1226,19 +1296,29 @@ TaggedNodePointer getChildGeq(Node16 *self, int child) { return nullptr; } -#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK) - uint32_t bitfield = - find_ge_16(self->index, child) & ((1 << self->numChildren) - 1); +#ifdef HAS_AVX + __m128i key_vec = _mm_set1_epi8(child); + __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) - // The index load is done in assembly (find_ge_16) so that reading the - // potentially-indeterminate unused index bytes is well-defined. - uint64_t bitfield = find_ge_16(self->index, child); + uint8x16_t indices; + memcpy(&indices, self->index, sizeof(self->index)); + // 0xff for each leq + auto results = vcleq_u8(vdupq_n_u8(child), indices); assume(self->numChildren <= Node16::kMaxNodes); uint64_t mask = self->numChildren == 16 ? uint64_t(-1) : (uint64_t(1) << (self->numChildren * 4)) - 1; - bitfield &= mask; + // 0xf for each 0xff (within mask) + uint64_t bitfield = + vget_lane_u64( + vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(results), 4)), + 0) & + mask; return bitfield == 0 ? nullptr : self->children[std::countr_zero(bitfield) / 4]; #else @@ -2094,11 +2174,14 @@ bool scan16(const InternalVersionT *vs, const uint8_t *is, int begin, int end, #ifdef HAS_ARM_NEON - // The index load is done in assembly (mask_in_range_16) so that reading - // potentially-indeterminate unused index bytes is well-defined. `vs` slots - // beyond the in-use range are always initialized (to zero) by the allocator, - // so the version compare below stays in C++. - uint64_t mask = mask_in_range_16(is, begin, end); + uint8x16_t indices; + memcpy(&indices, is, 16); + // 0xff for each in bounds + auto results = + vcltq_u8(vsubq_u8(indices, vdupq_n_u8(begin)), vdupq_n_u8(end - begin)); + // 0xf for each 0xff + uint64_t mask = vget_lane_u64( + vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(results), 4)), 0); uint64_t compared = vget_lane_u64( vreinterpret_u64_u8(vshrn_n_u16( @@ -2107,9 +2190,13 @@ bool scan16(const InternalVersionT *vs, const uint8_t *is, int begin, int end, 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) { @@ -2247,11 +2334,14 @@ bool checkMaxBetweenExclusiveImpl(Node16 *n, int begin, int end, #ifdef HAS_ARM_NEON - // The index load is done in assembly (mask_in_range_16) so that reading the - // potentially-indeterminate unused index bytes is well-defined. The unused - // childMaxVersion slots are always initialized (to zero) by the allocator, - // so the version compare below stays in C++. - uint64_t mask = mask_in_range_16(self->index, begin, end); + uint8x16_t indices; + memcpy(&indices, self->index, 16); + // 0xff for each in bounds + auto results = + vcltq_u8(vsubq_u8(indices, vdupq_n_u8(begin)), vdupq_n_u8(end - begin)); + // 0xf for each 0xff + uint64_t mask = vget_lane_u64( + vreinterpret_u64_u8(vshrn_n_u16(vreinterpretq_u16_u8(results), 4)), 0); mask &= self->numChildren == 16 ? uint64_t(-1) @@ -2271,10 +2361,15 @@ bool checkMaxBetweenExclusiveImpl(Node16 *n, int begin, int end, 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) & - ((1 << self->numChildren) - 1); + __m128i indices; + 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; } diff --git a/simd.h b/simd.h deleted file mode 100644 index ffa4f8b..0000000 --- a/simd.h +++ /dev/null @@ -1,48 +0,0 @@ -#pragma once - -#include - -#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); -} - -#elif defined(__aarch64__) && !defined(USE_SIMD_FALLBACK) - -// SIMD operations on potentially-indeterminate Node16::index[16] bytes. -// Implemented in file-level assembly (simd_aarch64.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. -// -// AArch64 has no pmovmskb-equivalent, so (unlike x86-64) each function returns -// a 64-bit "nibble mask": nibble i (bits [4i, 4i+4)) is 0xf iff the condition -// holds at index i. Bit (4i + 3) is the high bit of byte i's result. Callers -// locate a set lane with std::countr_zero(bitfield) / 4 and mask the valid -// lanes with (uint64_t(1) << (numChildren * 4)) - 1. - -extern "C" { -// Returns nibble i = 0xf iff idx[i] == key -uint64_t find_eq_16(const uint8_t idx[16], uint8_t key); -// Returns nibble i = 0xf iff idx[i] >= child -uint64_t find_ge_16(const uint8_t idx[16], uint8_t child); -// Returns nibble i = 0xf iff begin <= idx[i] < end -uint64_t mask_in_range_16(const uint8_t idx[16], uint8_t begin, uint8_t end); -} - -#endif diff --git a/simd_aarch64.S b/simd_aarch64.S deleted file mode 100644 index d1fd561..0000000 --- a/simd_aarch64.S +++ /dev/null @@ -1,84 +0,0 @@ -// SIMD operations on potentially-indeterminate Node16::index[16] bytes. -// Written in assembly because loading and operating on indeterminate values -// is undefined behavior in C++ ([basic.indet]) but well-defined in assembly. -// The caller is responsible for masking the returned bitfield to -// [0, numChildren) before using it. -// -// Unlike x86-64 (which has pmovmskb), AArch64 has no single instruction that -// produces a 1-bit-per-byte mask. Each function therefore returns a 64-bit -// "nibble mask" in x0: nibble i (bits [4i, 4i+4)) is 0xf iff the condition -// holds at index i. Bit (4i + 3) is the high bit of byte i's result. Callers -// locate a set lane with countr_zero(bitfield) / 4 and mask the valid lanes -// with (uint64_t(1) << (numChildren * 4)) - 1. -// -// AArch64 AAPCS: -// x0 = const uint8_t *idx (16 bytes, may contain indeterminate data) -// w1 = uint8_t key (find_eq_16, find_ge_16) -// w1 = uint8_t begin (mask_in_range_16) -// w2 = uint8_t end (mask_in_range_16) -// -// These functions are only ever called directly (never indirectly), so they -// do not need BTI landing pads; the object is still marked BTI/PAC/GCS-aware -// below so a -z force-bti link keeps BTI enabled for the whole binary. - - .text - -// uint64_t find_eq_16(const uint8_t idx[16], uint8_t key) -// nibble i = 0xf iff idx[i] == key - .globl find_eq_16 - .type find_eq_16, %function -find_eq_16: - dup v1.16b, w1 // broadcast key - ldr q0, [x0] // load 16 bytes (may be indeterminate) - cmeq v0.16b, v0.16b, v1.16b // 0xff for each match - shrn v0.8b, v0.8h, 4 // pack 16 byte-flags into 8 nibble-pairs - umov x0, v0.d[0] - ret - .size find_eq_16, .-find_eq_16 - -// uint64_t find_ge_16(const uint8_t idx[16], uint8_t child) -// nibble i = 0xf iff idx[i] >= child (unsigned) -// cmhs gives unsigned ">=" (higher-or-same): Vd = Vn >= Vm. - .globl find_ge_16 - .type find_ge_16, %function -find_ge_16: - dup v1.16b, w1 // broadcast child - ldr q0, [x0] // load 16 bytes - cmhs v0.16b, v0.16b, v1.16b // 0xff where idx[i] >= child (unsigned) - shrn v0.8b, v0.8h, 4 - umov x0, v0.d[0] - ret - .size find_ge_16, .-find_ge_16 - -// uint64_t mask_in_range_16(const uint8_t idx[16], uint8_t begin, uint8_t end) -// nibble i = 0xf iff begin <= idx[i] < end (unsigned, wrapping arithmetic) -// Logic: (idx[i] - begin) < (end - begin), valid when end - begin < 256. -// cmhi gives unsigned ">" (higher): Vd = Vn > Vm. We want -// (end - begin) > (idx - begin), so Vn = (end - begin). - .globl mask_in_range_16 - .type mask_in_range_16, %function -mask_in_range_16: - dup v1.16b, w1 // broadcast begin - dup v2.16b, w2 // broadcast end - ldr q0, [x0] // load 16 bytes - sub v0.16b, v0.16b, v1.16b // idx - begin (wrapping) - sub v2.16b, v2.16b, v1.16b // end - begin (range size) - cmhi v0.16b, v2.16b, v0.16b // 0xff where (end-begin) > (idx-begin) - shrn v0.8b, v0.8h, 4 - umov x0, v0.d[0] - ret - .size mask_in_range_16, .-mask_in_range_16 - - // Declare AArch64 branch-protection compatibility, matching what the - // compiler emits for -mbranch-protection=standard (BTI + PAC + GCS). This - // keeps the object indistinguishable from C/C++ translation units for - // linkers enforcing BTI (-z force-bti). The functions above are only ever - // called directly, so they need no BTI landing pads; PAC/GCS compatibility - // holds trivially since they use no stack. - - .aeabi_subsection aeabi_feature_and_bits, optional, ULEB128 - .aeabi_attribute Tag_Feature_BTI, 1 - .aeabi_attribute Tag_Feature_PAC, 1 - .aeabi_attribute Tag_Feature_GCS, 1 - - .section .note.GNU-stack,"",@progbits diff --git a/simd_x86_64.S b/simd_x86_64.S deleted file mode 100644 index 74f7741..0000000 --- a/simd_x86_64.S +++ /dev/null @@ -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 \ No newline at end of file From 6d9810fe7f9deaf9cd83f053d029924379124a32 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Mon, 3 Aug 2026 17:38:21 -0400 Subject: [PATCH 2/2] Poison Node48 reverseIndex slot on erase When a Node48 loses a child, the vacated reverseIndex slot would otherwise remain defined, letting valgrind miss reads that rely on the stale value. Mark it undefined, mirroring the existing poisoning of the vacated children slot. --- ConflictSet.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ConflictSet.cpp b/ConflictSet.cpp index a82d0d1..5572dd3 100644 --- a/ConflictSet.cpp +++ b/ConflictSet.cpp @@ -2025,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);