Skip to content

Vectorize checked multiplication by reporting evidence, not a comparison - #9210

Merged
connortsui20 merged 1 commit into
developfrom
ct/unsigned-mul-evidence
Aug 5, 2026
Merged

Vectorize checked multiplication by reporting evidence, not a comparison#9210
connortsui20 merged 1 commit into
developfrom
ct/unsigned-mul-evidence

Conversation

@connortsui20

@connortsui20 connortsui20 commented Aug 5, 2026

Copy link
Copy Markdown
Member

Replaces the per-lane overflow comparison in checked multiplication with the bits the narrow product discards. An unsigned checked multiply widens both operands, multiplies, and compares the product against the narrow maximum, and LLVM canonicalizes that into llvm.umul.with.overflow, which has no vector lowering. Eight spellings of the check compile to the same scalar code. The lane now returns the discarded half of the true product, which is non-zero on exactly the lanes that overflow, and map_checked_into OR-reduces it. Overflow detection is unchanged for every input.

The narrow signed widths keep their two-sided range check, which LLVM does not fold, so they measure neutral below. map_checked_into asserts that the evidence is no wider than the value, because a wider word puts the reduction rather than the arithmetic in charge of how many lanes a vector covers. The three kernel wrappers also relax from #[inline(always)] to #[inline], which measures the same.

Open question: do the 64-bit widths need u64 evidence rather than bool? There is no vector integer multiply at 64-bit width below AVX-512DQ, so the width is possibly not load-bearing on the CodSpeed runner.

Follow-up: prove from cached Stat::Min and Stat::Max that a multiply cannot overflow and skip the check entirely, the way values_fit_in already does for narrowing casts in cast.rs. Against a build with the check compiled out, that is worth a further 1.9x at u8 and nothing at u32.

Benchmark results

binary_ops, 65536 rows, divan fastest of 100 samples, interleaved A/B over two rounds against develop on an Apple M4 Max. add_i64_nonnull, div_i64_nonnull and eq_i64_constant are controls over untouched code.

benchmark before after
mul_u8_nonnull 10.74 µs 1.332 µs 8.1x
mul_u16_nonnull 11.58 µs 1.707 µs 6.8x
mul_u32_nonnull 12.29 µs 3.457 µs 3.6x
mul_u64_nonnull 12.41 µs 6.999 µs 1.8x
mul_i64_nonnull 13.20 µs 8.082 µs 1.6x
mul_i8_nonnull 1.457 µs 1.457 µs neutral
mul_i16_nonnull 2.624 µs 2.624 µs neutral
mul_i32_nonnull 3.957 µs 3.999 µs neutral
mul_i32_nullable 4.540 µs 4.540 µs neutral
mul_i32_constant 3.915 µs 3.957 µs neutral
add_i64_nonnull 6.791 µs 6.832 µs control
div_i64_nonnull 18.79 µs 17.54 µs control
eq_i64_constant 3.582 µs 3.582 µs control

mul.with.overflow falls from 20 occurrences to 4 across the checked kernels, and OR reductions appear at v16i8, v8i16 and v4i32, each the width of its own operand. The remaining 4 are the scalar constant-folding path.

CodSpeed runs on x86, where the 64-bit rows are expected to differ from the aarch64 numbers above.

@connortsui20 connortsui20 added the changelog/performance A performance improvement label Aug 5, 2026
@connortsui20
connortsui20 force-pushed the ct/unsigned-mul-evidence branch from ae7a468 to ab44cb7 Compare August 5, 2026 18:18
@connortsui20
connortsui20 changed the base branch from develop to ct/mul-u64-benchmark August 5, 2026 18:18
@codspeed-hq

codspeed-hq Bot commented Aug 5, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚡ 3 improved benchmarks
❌ 1 regressed benchmark
✅ 1891 untouched benchmarks
⏩ 43 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation decompress[u64, (1000, 16)] 65.2 µs 73.7 µs -11.5%
Simulation mul_u8_nonnull 405.3 µs 165.8 µs ×2.4
Simulation mul_u16_nonnull 501.5 µs 269.7 µs +85.94%
Simulation mul_u32_nonnull 722.3 µs 487.3 µs +48.24%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/unsigned-mul-evidence (ec9c9b1) with develop (8316455)

Open in CodSpeed

Footnotes

  1. 43 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

connortsui20 added a commit that referenced this pull request Aug 5, 2026
Adds a `mul_u64_nonnull` case to `binary_ops`. The suite covers `Mul` at
every signed width and at `u8`, `u16` and `u32`, but not at `u64`, which
takes a different overflow check because it has no wider native type to
widen into.

This lands separately so that CodSpeed records a baseline on `develop`
before #9210 changes that check. Measured on that PR, the width gains
1.8x, which no existing benchmark would have caught.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Base automatically changed from ct/mul-u64-benchmark to develop August 5, 2026 18:47
@connortsui20
connortsui20 force-pushed the ct/unsigned-mul-evidence branch 5 times, most recently from 5374340 to b74dc96 Compare August 5, 2026 21:03
An unsigned checked multiply is a widening multiply plus a comparison of
the product against the narrow maximum. LLVM canonicalizes exactly that
shape into `llvm.umul.with.overflow`, which has no vector lowering, so
the comparison costs the loop its vectorization. Eight spellings of the
check all fold to the same scalar code, and a twelve line program with no
Vortex in it reproduces it. The narrow signed widths escape only because
their two-sided range check does not match the same pattern, which is why
`mul_u8` costs eight times what `mul_i8` does for the same code.

So the lane no longer compares. It hands back the bits the narrow product
discarded, which are non-zero on exactly the lanes that overflowed, and
`map_checked_into` OR-reduces them. The 64-bit widths report the high
half of the true product, xored with the sign extension of the kept half
when signed. Carrying evidence wider than the operand would put the
reduction rather than the arithmetic in charge of how many lanes a vector
covers, so `map_checked_into` asserts that bound at compile time.

`mul_error` is deleted rather than left beside its replacement, and each
`mul_failure` is held against `checked_mul` in tests. The three kernel
wrappers relax from `#[inline(always)]` to `#[inline]`. Dropping the
attribute outright costs the constant-operand benchmarks multiples,
because the captured scalar stops flattening into a register, but forcing
the inline measures the same as hinting it.

Interleaved A/B, divan fastest of 100 samples, 65536 rows, two rounds on
an Apple M4 Max. Signed widths below 64 bits are neutral, and
`add_i64_nonnull` and `eq_i64_constant` are held as controls:

    mul_u8_nonnull      10.74 us -> 1.332 us   8.1x
    mul_u16_nonnull     11.58 us -> 1.707 us   6.8x
    mul_u32_nonnull     12.29 us -> 3.457 us   3.6x
    mul_u64_nonnull     12.41 us -> 6.999 us   1.8x
    mul_i64_nonnull     13.20 us -> 8.082 us   1.6x

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the ct/unsigned-mul-evidence branch from b74dc96 to ec9c9b1 Compare August 5, 2026 21:14
@connortsui20
connortsui20 marked this pull request as ready for review August 5, 2026 21:21
@connortsui20
connortsui20 enabled auto-merge (squash) August 5, 2026 21:39
fn sub_error(self, rhs: Self) -> bool;
fn mul_value(self, rhs: Self) -> Self;
fn mul_error(self, rhs: Self) -> bool;
fn mul_failure(self, rhs: Self) -> Self::MulFailure;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this stands out quite a lot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

but I guess you only need this for multiplication since division won't work like this and add/sub only needs a bool

@connortsui20
connortsui20 merged commit 4c44e04 into develop Aug 5, 2026
95 of 97 checks passed
@connortsui20
connortsui20 deleted the ct/unsigned-mul-evidence branch August 5, 2026 22:10
connortsui20 pushed a commit that referenced this pull request Aug 6, 2026
Brings `develop` through #9228. Three files conflicted, all under
`scalar_fn/fns/binary/numeric/`, because #9210 optimized the
very kernels this branch replaced with the row framework.

`primitive.rs` keeps this branch's version. #9210 arrived independently at the
same design, and its four `mul_failure` bodies are identical to this branch's
width for width: the evidence is the discarded high half of the widened product,
so the lane never compares and LLVM cannot fold the check into
`llvm.umul.with.overflow`. What `develop` keeps and the row path does not need is
`CHECKED_VALUE_LOOP`, `DIV_CHECKS_IN_VALUE_LOOP`, `div_checked` and
`CheckedPrimitiveOp::checked`, which exist to choose between its split and
early-exit lane kernels; a row kernel produces the value and the evidence in one
pass and has no choice to make. `develop`'s named intermediates in the signed
64-bit body are adopted.

`checked.rs` takes `develop`'s `checked_lanes`, including its rewritten docs and
the relaxation from `#[inline(always)]` to `#[inline]`. `checked_apply_lanes` and
the `Failure` trait beside it are dropped: the split value/evidence pass they
served is now `CheckedSink`, and decimal, their only other neighbour, uses
`checked_lanes` alone.

`tests.rs` takes both sides. #9210's `test_multiply_overflow_boundaries` drives
`execute_numeric` end to end rather than the kernels directly, so its 20 cases
cross-check the row path against `develop`'s intent at every overflow boundary of
every width formula. They pass unmodified.

`NUMERIC_ROWFN_PLAN.md` is corrected for what this does to its benchmark table,
which it had already predicted: the unsigned multiply rows compared the row
framework against a defect `develop` has now fixed, so they are not a `RowFn` win
and the re-measurement they asked for is now like-for-like.

Verification: 3355 vortex-array, 179 vortex-tensor and 241 vortex-geo tests, 73
doctests, clippy over those three plus vortex-compute with `--all-targets
--all-features`, nightly fmt, `git diff --check`, and builds of vortex-file,
vortex-datafusion, vortex-layout, vortex-scan, vortex-compute and
vortex-btrblocks. `lance-bench` and `vortex-nvcomp` do not build in this
environment: `protoc` and CUDA are absent, on `develop` as well.

Signed-off-by: "Connor Tsui" <connor@spiraldb.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014mwiAahcxc5xBfTrhDK11L
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/performance A performance improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants