mbutrovich commented on PR #11818: URL: https://github.com/apache/arrow-rs/pull/11818#issuecomment-6580310009
Thanks for the review @alamb! > My only concern is that this particular optimization is too overfit to the current aarch64 / llvm code generation. Any loop that LLVM vectorizes depends on its codegen, and `main`'s `u64::count_ones` sum does too: it becomes `cnt.16b`, `udot`, and `uadalp` only because LLVM vectorizes it. As I read it, that's the trade-off arrow-rs accepted when it replaced explicit SIMD with auto-vectorization (#5032, #5184), as [CONTRIBUTING.md](https://github.com/apache/arrow-rs/blob/e2c48ef0dd83fa464d643e25f8e9f9177ca7316e/arrow/CONTRIBUTING.md?plain=1#L112-L122) describes. This PR's 64-byte loop compiles to the same instructions on every toolchain from rustc 1.88 (the MSRV, LLVM 20) to nightly (LLVM 23), but a later LLVM could change it or `main`'s loop, and a benchmark regression check would catch either. Should we run the benchmarks on each `rust-toolchain.toml` bump (such as #11618) and add a benchmark check to the release checklist, the way #11631 added Miri? > Also, when I ran this on an x86_64 is also measures significantly faster (though given the overall timing is in the ns range, I do wonder if there are other effects going on) On x86_64, `count_ones_words` is the same `u64::count_ones` sum as `main`. I compiled the commit before this change (f6c3989363) and the PR head (110c2e8254) for `x86_64-unknown-linux-gnu` with the release profile, which is what `cargo bench` uses here. `BooleanArray::true_count`, `Buffer::count_set_bits_offset`, and `UnalignedBitChunk::count_ones` are identical in both, apart from label numbers. The `nulls_all_true` cases also moved 3% to 10% in your run, and that path never calls `count_ones`. So as I read it, the x86_64 differences come from code layout, not from this change. -- 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]
