From b4589c0ec3b67557a47715dd5f0b13201e73f4e8 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Thu, 27 Aug 2026 14:54:51 -0400 Subject: [PATCH 1/2] Clarify Boolean collection code generation Signed-off-by: Connor Tsui --- vortex-buffer/src/bit/buf.rs | 49 ++++++++++++++++++-------------- vortex-buffer/src/bit/buf_mut.rs | 30 +++---------------- 2 files changed, 31 insertions(+), 48 deletions(-) diff --git a/vortex-buffer/src/bit/buf.rs b/vortex-buffer/src/bit/buf.rs index 2a81b00d5c1..ae7efc8ff82 100644 --- a/vortex-buffer/src/bit/buf.rs +++ b/vortex-buffer/src/bit/buf.rs @@ -191,39 +191,44 @@ impl BitBuffer { } } - /// Invokes `f` with indexes `0..len` collecting the boolean results into a new [`BitBuffer`]. + /// Collects `len` Boolean values from `f` into a packed [`BitBuffer`]. /// - /// `f` is invoked exactly once per index, in ascending order, and the results are packed - /// with the baseline SIMD byte→bit instruction of the target. + /// Calls `f` exactly once for each index in `0..len`, in order. + /// + /// # Code generation + /// + /// Each full 64-value chunk is materialized as a byte-per-value `[bool; 64]`, then packed into + /// one bitmap word. For simple predicates, LLVM vectorizes the loop and removes the physical + /// stack array. On AVX-512, it still combines the comparison masks, expands the result into 64 + /// `0` or `1` bytes with `vpbroadcastq` and `vmovdqu8`, then recreates the mask with + /// `vptestmb`. + /// [LLVM issue #219235](https://github.com/llvm/llvm-project/issues/219235) tracks replacing + /// that round trip with a direct `kmovq` store. The conversion is per chunk. This method does + /// not create a full-column byte buffer. /// /// # Performance /// - /// The packing is a few instructions per 64 bits, so evaluating `f` is usually the - /// bottleneck. In particular, a bounds-checked slice access in `f` (`|i| values[i] > x`) - /// blocks vectorization of the gather and can cost ~10x the packing itself. Since `f` only - /// ever sees indices `0..len`, callers reading from a slice with `len <= values.len()` may - /// soundly use `|i| unsafe { *values.get_unchecked(i) }`. + /// The packing kernel is selected at compile time, so the fill-and-pack loop can inline into + /// its caller. A retained bounds check inside `f` can prevent vectorization. A caller that + /// proves `len <= values.len()` can use `unsafe { *values.get_unchecked(i) }` because this + /// method only passes indices in `0..len`. /// - /// Prefer this entry point for every predicate. Only switch to - /// [`Self::collect_bool_multiversioned`] after carefully checking that your specific `f` - /// meets its contract (a trivially cheap, bounds-check-free gather or comparison) — - /// ideally with a benchmark. + /// Use this method for general predicates. Use [`Self::collect_bool_multiversioned`] only for + /// the specialized predicates described there. #[inline] pub fn collect_bool bool>(len: usize, f: F) -> Self { BitBufferMut::collect_bool(len, f).freeze() } - /// Like [`Self::collect_bool`], but compiles the packing loop — with `f` inside it — once - /// per CPU feature level (AVX-512BW/AVX2/baseline) and selects a clone by runtime feature - /// detection. + /// Collects Boolean values with a fill-and-pack loop selected for the current CPU. + /// + /// This has the same callback contract as [`Self::collect_bool`]. On x86-64, it compiles the + /// complete loop, including `f`, for AVX-512BW, AVX2, and the statically enabled fallback, then + /// selects a version at runtime. Each wider version is a `#[target_feature]` function, so Rust + /// cannot inline it into a caller compiled without those features. /// - /// Calling this asserts that `f` is small and simple enough (e.g. a bounds-check-free slice - /// gather or comparison) that duplicating it per feature level and paying a - /// `#[target_feature]` call boundary beats inlining it once into your function. For any - /// non-trivial `f` that assertion is false — the boundary deoptimizes the predicate — so - /// unless you have carefully checked (ideally benchmarked) that your specific `f` - /// qualifies, use [`Self::collect_bool`]. See - /// [`collect_bool_words_multiversioned`](crate::bit::collect_bool_words_multiversioned). + /// Use this method only for a small, bounds-check-free predicate whose wider loop has been + /// benchmarked. Use [`Self::collect_bool`] for general predicates. #[inline] pub fn collect_bool_multiversioned bool>(len: usize, f: F) -> Self { BitBufferMut::collect_bool_multiversioned(len, f).freeze() diff --git a/vortex-buffer/src/bit/buf_mut.rs b/vortex-buffer/src/bit/buf_mut.rs index a1bb0c82c04..d7dc59e7fe6 100644 --- a/vortex-buffer/src/bit/buf_mut.rs +++ b/vortex-buffer/src/bit/buf_mut.rs @@ -189,39 +189,17 @@ impl BitBufferMut { } } - /// Invokes `f` with indexes `0..len` collecting the boolean results into a new `BitBufferMut` + /// Mutable-buffer form of [`BitBuffer::collect_bool`]. /// - /// `f` is invoked exactly once per index, in ascending order, and the results are packed - /// with the baseline SIMD byte→bit instruction of the target. - /// - /// # Performance - /// - /// The packing is a few instructions per 64 bits, so evaluating `f` is usually the - /// bottleneck. In particular, a bounds-checked slice access in `f` (`|i| values[i] > x`) - /// blocks vectorization of the gather and can cost ~10x the packing itself. Since `f` only - /// ever sees indices `0..len`, callers reading from a slice with `len <= values.len()` may - /// soundly use `|i| unsafe { *values.get_unchecked(i) }`. - /// - /// Prefer this entry point for every predicate. Only switch to - /// [`Self::collect_bool_multiversioned`] after carefully checking that your specific `f` - /// meets its contract (a trivially cheap, bounds-check-free gather or comparison) — - /// ideally with a benchmark. + /// Calls `f` in the same order and uses the same packing path. #[inline] pub fn collect_bool bool>(len: usize, f: F) -> Self { Self::collect_words(len, |words| collect_bool_words(words, len, f)) } - /// Like [`Self::collect_bool`], but compiles the packing loop — with `f` inside it — once - /// per CPU feature level (AVX-512BW/AVX2/baseline) and selects a clone by runtime feature - /// detection. + /// Mutable-buffer form of [`BitBuffer::collect_bool_multiversioned`]. /// - /// Calling this asserts that `f` is small and simple enough (e.g. a bounds-check-free slice - /// gather or comparison) that duplicating it per feature level and paying a - /// `#[target_feature]` call boundary beats inlining it once into your function. For any - /// non-trivial `f` that assertion is false — the boundary deoptimizes the predicate — so - /// unless you have carefully checked (ideally benchmarked) that your specific `f` - /// qualifies, use [`Self::collect_bool`]. See - /// [`collect_bool_words_multiversioned`]. + /// Calls `f` in the same order and uses the same packing path. #[inline] pub fn collect_bool_multiversioned bool>(len: usize, f: F) -> Self { Self::collect_words(len, |words| { From 81fa25268c49208de78075621ac85cd35a9d6fa2 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Thu, 27 Aug 2026 15:06:16 -0400 Subject: [PATCH 2/2] Name Boolean packing functions in docs Signed-off-by: Connor Tsui --- vortex-buffer/src/bit/buf.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/vortex-buffer/src/bit/buf.rs b/vortex-buffer/src/bit/buf.rs index ae7efc8ff82..1d7f42b70fb 100644 --- a/vortex-buffer/src/bit/buf.rs +++ b/vortex-buffer/src/bit/buf.rs @@ -197,21 +197,22 @@ impl BitBuffer { /// /// # Code generation /// - /// Each full 64-value chunk is materialized as a byte-per-value `[bool; 64]`, then packed into - /// one bitmap word. For simple predicates, LLVM vectorizes the loop and removes the physical + /// `collect_bool_words` calls `collect_bool_words_inline`, which selects a `pack_bool_word_*` + /// kernel at compile time and passes it to `collect_bool_words_with`. That shared word loop + /// materializes each full 64-value chunk as a byte-per-value `[bool; 64]`, then passes it to the + /// selected kernel. For simple predicates, LLVM vectorizes the loop and removes the physical /// stack array. On AVX-512, it still combines the comparison masks, expands the result into 64 - /// `0` or `1` bytes with `vpbroadcastq` and `vmovdqu8`, then recreates the mask with - /// `vptestmb`. + /// `0` or `1` bytes with `vpbroadcastq` and `vmovdqu8`, then recreates the mask with `vptestmb`. /// [LLVM issue #219235](https://github.com/llvm/llvm-project/issues/219235) tracks replacing /// that round trip with a direct `kmovq` store. The conversion is per chunk. This method does /// not create a full-column byte buffer. /// /// # Performance /// - /// The packing kernel is selected at compile time, so the fill-and-pack loop can inline into - /// its caller. A retained bounds check inside `f` can prevent vectorization. A caller that - /// proves `len <= values.len()` can use `unsafe { *values.get_unchecked(i) }` because this - /// method only passes indices in `0..len`. + /// `collect_bool_words_inline` and `collect_bool_words_with` can inline into the caller, so LLVM + /// sees `f`, the fill loop, and the packing kernel together. A retained bounds check inside `f` + /// can prevent vectorization. A caller that proves `len <= values.len()` can use + /// `unsafe { *values.get_unchecked(i) }` because this method only passes indices in `0..len`. /// /// Use this method for general predicates. Use [`Self::collect_bool_multiversioned`] only for /// the specialized predicates described there. @@ -222,10 +223,11 @@ impl BitBuffer { /// Collects Boolean values with a fill-and-pack loop selected for the current CPU. /// - /// This has the same callback contract as [`Self::collect_bool`]. On x86-64, it compiles the - /// complete loop, including `f`, for AVX-512BW, AVX2, and the statically enabled fallback, then - /// selects a version at runtime. Each wider version is a `#[target_feature]` function, so Rust - /// cannot inline it into a caller compiled without those features. + /// This has the same callback contract as [`Self::collect_bool`]. On x86-64, + /// `collect_bool_words_multiversioned` selects `collect_bool_words_avx512`, + /// `collect_bool_words_avx2`, or `collect_bool_words_inline` at runtime. Each wider version is a + /// `#[target_feature]` function, so Rust cannot inline it into a caller compiled without those + /// features. /// /// Use this method only for a small, bounds-check-free predicate whose wider loop has been /// benchmarked. Use [`Self::collect_bool`] for general predicates.