sdf-jkl opened a new pull request, #11979: URL: https://github.com/apache/arrow-rs/pull/11979
Draft benchmarking follow-up to #11467 and [this discussion](https://github.com/apache/arrow-rs/pull/11467#issuecomment-6481711357). This branch is stacked on its head, `46ab603e26b22d444203d660602605e53a2a8733`. Until the parent is merged and this branch is rebased, the main-relative PR diff includes the parent changes; [the incremental diff](https://github.com/sdf-jkl/arrow-rs/compare/46ab603e26b22d444203d660602605e53a2a8733...4641eda8ce81922fa2ce9ebf5143700a994b73c0) is just the signed-arithmetic rewrite. # Which issue does this PR close? Relates to #11421; depends on #11467. # Rationale for this change The signed high-product expression can generate expensive scalar or vector multiplication sequences. This draft tries an equivalent unsigned high product with explicit sign correction for both `i32` and `i64`, and requests measurements against #11467's exact head before deciding whether the tradeoffs are worthwhile. # What changes are included in this PR? For the always-negative multiplier `m`, replace `n + signed_high(m * n)` with `unsigned_high(bits(m) * bits(n)) - ((n >> (BITS - 1)) & m)`, using wrapping subtraction. The signed macro gains matching unsigned narrow/wide types. Constructors, quotient rounding, remainder calculation, public APIs and the benchmark source are unchanged relative to #11467. The correction follows [libdivide's signed high-product implementation](https://github.com/ridiculousfish/libdivide/blob/master/libdivide.h#L2530-L2537), specialized so its `-n` cancels the existing `+n`. This follows [devanbenz and mbutrovich's codegen discussion](https://github.com/apache/arrow-rs/pull/11467#discussion_r4380694099); [mightsleep's absolute-dividend approach](https://github.com/apache/arrow-rs/pull/11467#pullrequestreview-5658920404) is a related, different alternative. # Are these changes tested? The existing 376 `arrow-buffer` unit tests pass. An external Hegel harness compared all four divisor types with Rust's integer operations, including negative divisors, boundary values, overflow behavior, const evaluation, and batches, with overflow checks on/off across five x86-64 target settings. No new test dependency or benchmark cases are added here. Scoped Clippy, formatting and spelling checks pass; the full workspace suite was not run locally. All 48 unchanged `divisor` benchmark cases were run on an AMD Ryzen AI 9 HX PRO 470 with Rust 1.99.1 / LLVM 23.1.1 and thin LTO. Below are original/rewrite time ratios for divisors 7 and 13; above 1 is faster: | Target | i32 division | i32 remainder | i64 division | i64 remainder | |---|---:|---:|---:|---:| | x86-64 | 1.72–1.89 | 1.75–1.79 | 1.38–1.39 | 0.86–0.94 | | x86-64-v2 | 1.94–2.16 | 1.76–1.99 | 1.39–1.55 | 0.85–0.92 | | x86-64-v3 | 0.96–1.02 | 1.15–1.18 | 1.91–1.97 | 1.53–1.54 | | x86-64-v4 | 0.99–1.01 | 0.97 | 1.79–1.85 | 1.77–1.78 | | native | 0.91–0.95 | 0.92–1.01 | 1.36–1.43 | 1.52–1.57 | These are exploratory measurements: four interleaved processes per case, 0.25 s warmup / 0.75 s measurement, with some substantial repeat noise. Unchanged controls also regressed (for example AVX2 `u32 % 7` by about 27%); those effects are unresolved. The draft is for independent bot measurements, not a claim of a universal improvement. Bot requests pin both commits and apply identical flags to both sides. # Are there any significant user-facing changes? No additional public API or intended behavior changes relative to #11467. AI assistance was used to develop and test this experiment. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
