Collapse the checked-arithmetic macros into one integer body - #9248
Merged
Conversation
robert3005
approved these changes
Aug 6, 2026
connortsui20
added a commit
that referenced
this pull request
Aug 6, 2026
…ngs (#9245) Progress towards #9128. Cleanup from a review of the `RowFn` API, the execution logic, and the scalar function implementations: deletes the `InputElement::decode_null_tolerant` overrides that only restated the default, shares `resolve_validity` between `execute_filtered` and the forced-strategy test seam, corrects the dense-retry comment, de-duplicates `l2_norm_row` into `vortex-tensor/src/utils.rs`, drops the now-callerless `map_checked_into`, and pins `geo` to `=0.31.0` because `contains_route` transcribes its `impl_contains_from_relate!` dispatch table and a patch can reshuffle that with no API change. The author-facing API is unchanged: every proposal that would have altered it was backed out, and `SCALAR_FN_HANDOFF.md` records which ones and why. The checked-arithmetic macro collapse this review also found is #9248 against `develop` instead, since `develop` carries the same duplication. <details> <summary>Checks</summary> ``` cargo test -p vortex-array -p vortex-compute -p vortex-geo -p vortex-tensor # 3838 passed, 0 failed cargo clippy -p vortex-array -p vortex-compute -p vortex-geo -p vortex-tensor --all-targets --all-features cargo +nightly fmt --all -- --check RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p vortex-array -p vortex-compute -p vortex-geo -p vortex-tensor ``` The optimized IR of every `visit_prepared_into` monomorph is unchanged from the pre-review commit. Runtime benchmarks were not usable as a gate on this host, where repeated pinned runs of the same binary disagreed by up to 4x. </details> Signed-off-by: Connor Tsui <connor@spiraldb.com> Co-authored-by: Claude <noreply@anthropic.com>
connortsui20
enabled auto-merge (squash)
August 6, 2026 20:38
connortsui20
added
changelog/chore
The four `CheckedArithmetic` macro arms were near-identical copies of the
same ~55-line trait impl. Only `mul_failure` genuinely varies per width, so
that is now the parameter of a single `impl_checked_integer!` and everything
else is written once.
The `overflowing_mul` arms existed only to specialize the 64-bit widths, and
both fold into the general form: `impl_checked_unsigned!(u64, widening_mul:
u128)` produces the same body the u64 arm spelled out, and `high_half_mul`
generalizes the signed one. Deriving the shift from `$ty` also retires the
`const { assert!(<$ty>::BITS == 64) }` that guarded the hardcoded `>> 64`,
so instantiating a width cannot silently keep another width's constants.
Pure refactor. The expanded impls differ from the previous ones only by an
alpha-rename of `self` to `lhs`, and the optimized LLVM IR of the checked
kernels at `-C target-cpu=x86-64-v3` is identical: same vector widths, same
reduction counts, and the same 309 `umul.with.overflow` / 109
`smul.with.overflow` sites crate-wide.
Also fixes a stale doc reference to `tests::test_i64_multiply_overflow_boundaries`,
which is named `test_multiply_overflow_boundaries`.
Signed-off-by: Connor Tsui <connor@spiraldb.com>
connortsui20
force-pushed
the
claude/collapse-checked-arith-macros
branch
from
August 7, 2026 14:59
7c0fc40 to
50e9c1d
Compare
Merging this PR will improve performance by 82.5%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #9210. The four
CheckedArithmeticmacro arms are near-identical copies of the same ~55-line impl, and onlymul_failurevaries between them, so that becomes the parameter of oneimpl_checked_integer!. Bothoverflowing_mularms existed only to specialize the 64-bit widths and fold into the general form, and deriving the shift from$tyretires theconst { assert!(<$ty>::BITS == 64) }that guarded the hardcoded>> 64. Pure refactor, no API change: the expanded impls differ from the previous ones only by an alpha-rename ofselftolhs, and the optimized IR of the checked kernels is identical.Refactor evidence and checks
Expanded impls compared per type with
cargo +nightly rustc --lib -- -Zunpretty=expandedat both revisions. Optimized LLVM IR at-C target-cpu=x86-64-v3, comparing everydefinetouching the checked kernels by vector width, reduction count, and overflow intrinsics: