GH-49449: [C++] Backport xsimd neon fix#49450
Conversation
|
|
AntoinePrv
force-pushed
the
xsimd-neon-backport
branch
from
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 Fix the build on MacOS with newest xsimd version. A fix for an internal bug in xsimd had been added in GH-49449 GH-49450 to boost performances. Now that the fix has been merged in xsimd 14.1.0, the internals have changed leading to the failure. The ported fix is kept for older xsimd versions until we can set a minimum xsimd version of 14.1.0. ### What changes are included in this PR? Remove a backported fix on xsimd versions that include it. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #49579 Authored-by: AntoinePrv <AntoinePrv@users.noreply.github.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
### 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 `unpack` routine 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. * GitHub Issue: apache#49449 Authored-by: AntoinePrv <AntoinePrv@users.noreply.github.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
### Rationale for this change Fix the build on MacOS with newest xsimd version. A fix for an internal bug in xsimd had been added in apacheGH-49449 apacheGH-49450 to boost performances. Now that the fix has been merged in xsimd 14.1.0, the internals have changed leading to the failure. The ported fix is kept for older xsimd versions until we can set a minimum xsimd version of 14.1.0. ### What changes are included in this PR? Remove a backported fix on xsimd versions that include it. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#49579 Authored-by: AntoinePrv <AntoinePrv@users.noreply.github.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
3 participants
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.