Repository navigation
Compare primitive values through the shared RowFn executor - #10676
connortsui20 wants to merge 8 commits into
Conversation
Merging this PR will regress 17 benchmarks4 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
dee1a57 to
3a38935
Compare
3a38935 to
33ccf9b
Compare
33ccf9b to
b837021
Compare
|
Edit: reopened as a draft. The controlled measurements showed substantial AVX2 gains as well as ARM and AVX-512 regressions. Those results justify holding the PR for further work, but do not establish that the migration has no benefit. The earlier closure treated those two conclusions as equivalent. #10741 isolates a useful float32 packing policy, and #10742 explores a smaller validity change. Neither replaces every benefit observed in the migration. Both remain independent drafts with their own measurement limits. #10675 and this PR are being rebased while preserving their dependency. The saved measurements are historical evidence, not validation of the rebased heads. |
b837021 to
3583fc2
Compare
Keep the primitive kernel focused on type and operator dispatch. Delegate borrowed inputs, constants, strict validity, and packed Boolean output to the lower stack layers while preserving Binary identity and floating-point total ordering. Replaces the comparison migration in #9979. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
A constant first operand vectorizes worse in the packed Boolean row loop, which made 8-bit constant-left comparisons slower than the native kernel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <github@robertk.io>
The multiversioned collector used by primitive comparison always filled one byte per row and then packed the bytes. For 64-bit lanes the narrowing to bytes costs more than accumulating the bits in a register: on every AVX2 row, and on AVX-512 when a batch constant leaves a single load per lane. Those shapes now accumulate bits directly. Every other shape, and aarch64, keeps the fill-and-pack loop, which stays faster there. The collector stays out of line. Inlined into a large caller, a statically enabled AVX-512 loop can spill its byte constant to a masked memory load that stalls when it aliases the output stores. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <github@robertk.io>
For a batch of at most 16Ki rows whose input validities are materialized Boolean arrays, one word-wise `and` costs less than building and optimizing a lazy `and` node. Larger batches keep the lazy node, which defers the work to a reader that may never come. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <github@robertk.io>
3583fc2 to
237dfc0
Compare
Select the packing loop from the `fearless_simd::Level` detected for the CPU instead of separate feature checks. Each level calls a `#[target_feature]` loop that takes the row callback by value: captured in a closure environment, the callback hides its batch-constant source from LLVM, which then does not unswitch the 8-bit loop. The selection rules are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <github@robertk.io>
A direct Boolean batch used to inspect its inputs before dispatch and decode them again inside the visit. Each step read the same lengths, dtypes and constant encodings, and the inspection cloned the argument dtypes into a list. Decode a direct batch in one pass after dispatch instead: classify literal constants by encoding, decode them from the scalar already read, and conjoin column validities, taking the first one unchanged. A batch that is not direct, including one with a null literal, a wrapped constant or a wrong row count, falls back to batch execution, which inspects it lazily and keeps its errors. Inputs that already hold their dtypes contiguously lend them to dispatch, so batch execution does not clone them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VT4SKVtrb3NMjXkMj4vW6S Signed-off-by: Robert Kruszewski <github@robertk.io>
Build the two operand dtypes as primitive values once and lend them to dispatch, and detect a constant left operand by its encoding instead of two dynamic downcasts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VT4SKVtrb3NMjXkMj4vW6S Signed-off-by: Robert Kruszewski <github@robertk.io>
`InputElement::decode_constant_ref` avoided one reference count per batch constant but measured no change in instructions per comparison. Direct batches decode constants through `decode_constant_scalar`, so the borrowed variant is only an indirection. Its default now decodes the owned array directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VT4SKVtrb3NMjXkMj4vW6S Signed-off-by: Robert Kruszewski <github@robertk.io>
Summary
Replaces the primitive comparison migration in #9979, using the shared executor from #10675. This PR depends on #10675 and has been replayed onto its rebased head
7c47d4bcfee3370684c1b22de999e61f66628afa.Reopened as a draft. The controlled measurements showed substantial AVX2 gains as well as ARM and AVX-512 regressions. They justify further work before merging, but do not establish that this migration has no benefit. #10741 isolates some float32 packing gains without this migration, while #10742 explores a smaller validity change. Neither is a complete replacement for every observed benefit here.
Changes
The private row kernel selects the primitive type and comparison operator. The framework owns borrowed inputs, constant handling, strict validity, and packed Boolean output. Removes the primitive operand wrapper and handwritten primitive kernels while preserving the public
Binaryidentity and Vortex's floating-point total ordering.Native Graviton3 measurements showed regressions when this migration used only #10673. Removing only the fused-execution patch while keeping all three foundations also slowed the tested comparisons. The dependency on #10675 keeps that execution work available when this migration merges.
Existing comparison tests exercise the shared executor. Source range-diff review confirms that the migration patch is unchanged by the rebase. Tests, formatting, and benchmarks were not rerun on the rebased head, so earlier measurements are not validation of this revision.