GH-49449: [C++] Backport xsimd neon fix#49450
Conversation
|
|
44a1a10 to
18ce22f
Compare
|
Can you explain what this is fixing in the PR description? |
|
Also, were you able to run some benchmarks? |
Not yet, I will do it overnight (I run some extensive benchmarks in a fork). I was however able to confirm from the assembly that this is the correct instructions we aimed to use. |
|
@pitrou I've run the benchmarks (though I have lost the old ones in the process) and it is much better. |
|
@github-actions crossbow submit -g cpp |
|
Revision: 18ce22f Submitted crossbow builds: ursacomputing/crossbow @ actions-471834543b |
|
@ursabot please benchmark lang=C++ |
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 18ce22f. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
|
I didn't realize we already cover neon via graviton machines, that's nice. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit dbbf7cf. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 26 possible false positives for unstable benchmarks that are known to sometimes produce them. |
|
@pitrou benchmarks look ok. |
|
Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit 18ce22f. There were 7 benchmark results indicating a performance regression:
The full Conbench report has more details. |
Rationale for this change
Performance in hot code.
There is a bug in xsimd where the rshift/lshift for Neon are implemented with a scalar loop instead of the appropriate SIMD intrinsics. This code path is core to the
unpackroutine in Parquet reads.xtensor-stack/xsimd#1266
What changes are included in this PR?
A bug backport.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.