Repository navigation
Big endian neon fixes - #884
Open
aaron-zetier wants to merge 6 commits into
Open
aaron-zetier wants to merge 6 commits into
aaron-zetier wants to merge 6 commits into
Conversation
The NEON index_min/index_max kernels initialized their lane-index vector
with a brace initializer, { 0, 1, 2, 3 }. On big-endian AArch64, GCC
fills a brace-initialized vector in the reverse of NEON lane order, so
lane 0 held index 3 and the kernels returned indices off by up to three
within a group of four. Load the indices from a static array with
vld1q_u32 instead, which gives the same lanes on either byte order.
Found on an aarch64_be target (GCC 11.2), where volk_profile flagged
neon and neonv8 of 32f_index_min_16u, 32fc_index_min_16u and
32fc_index_max_16u, and neonv8 of 32f_index_min_32u, 32fc_index_min_32u
and 32fc_index_max_32u. The float index_max kernels have the same bug
but pass that test: its input starts with 16 tied maxima, and the
tie-break hides the reversed lanes.
Signed-off-by: Aaron Krol <aaron.krol@zetier.com>
The neonv8 kernel built its [0, -0, 0, -0] sign mask from a brace-initialized float32x4_t. On big-endian AArch64 (seen with GCC 11.2), GCC lays that vector out in reverse NEON lane order, so the kernel negated the real parts instead of the imaginary parts. Load the mask with vld1q_f32 from a static array, which is correct on either byte order. Signed-off-by: Aaron Krol <aaron.krol@zetier.com>
Both neonv8 kernels reversed the bytes of each 32-bit word with vqtbl1q_u8 and a brace-initialized index vector. On big-endian AArch64 (seen with GCC 11.2), GCC lays that vector out in reverse NEON lane order, so the lookup reversed the order of the four words in each vector instead of the bytes within each word. vrev32q_u8 does the per-word byte reversal without a table and does not depend on byte order. Signed-off-by: Aaron Krol <aaron.krol@zetier.com>
The neonspiral kernel loaded and stored its path metrics and branch table through uint8x16_t pointer casts. On big-endian AArch64 (seen with GCC 11.2), GCC moves such vectors in the reverse of NEON lane order, so the lane-position dependent steps (vget_low/vget_high, vzip, vpmin, vgetq_lane) worked on the wrong lanes and the decisions differed from generic. Route the loads and stores through two macros that use vld1q_u8 and vst1q_u8 when __ARM_BIG_ENDIAN is defined. On little-endian they keep the pointer casts: vld1 has no immediate-offset addressing on 32-bit ARM, and switching to it there added address arithmetic and register spills to the loop. Signed-off-by: Aaron Krol <aaron.krol@zetier.com>
jdemel
reviewed
Oct 20, 2026
jdemel
left a comment
jdemel
left a comment
Contributor
There was a problem hiding this comment.
Thank you for your PR! And welcome :)
Since this doesn't seem to be an issue with any of our CI pipelines, we should have one that tests this case. Are you interested to add this to your PR?
No CI job built VOLK for a big-endian target with NEON, so lane-order bugs in the NEON kernels went unnoticed. Ubuntu has no big-endian AArch64 port or cross toolchain, so the job cross-compiles on an x86 runner with the Arm GNU Toolchain's aarch64_be-none-linux-gnu build, pinned by SHA-256 and cached, and runs ctest under qemu-aarch64_be, like the RVV job does for RISC-V. The new toolchain file points qemu at the toolchain's glibc sysroot and at its libstdc++, which sits outside the sysroot. Signed-off-by: Aaron Krol <aaron.krol@zetier.com>
The float index kernels' QA input began with 16 tied maxima of 1.0, so every lane held the maximum and every implementation returned index 0. A kernel that pairs values with the wrong lane indices gives that same answer, so the big-endian lane-order bugs in volk_32f_index_max_16u (neon, neonv8) and volk_32f_index_max_32u (neonv8) passed QA. Put a 0.0 first. The ties still cover every SIMD width, but the first maximum is now at index 1, partway into the first vector, where a lane-index mixup gives a different answer. Signed-off-by: Aaron Krol <aaron.krol@zetier.com>
Author
Thanks for the welcome, and yes I'm happy to add a CI job. I just (force)-pushed an update to the branch to put a CI job (as well as a tweak to the index test so its ties don't mask the lane-order bug) first in the chain with the code fixes on top of that. The first Ci commit flags the 13 instances described above, the second will flag the additional 3, then with the code fixes they all pass. Happy to tweak further if needed, just let me know. Thanks! |
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
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.
Several NEON kernels give wrong results on big-endian AArch64 (seen with GCC 11.2). The causes are two ways of creating a NEON vector that GCC lays out in the opposite lane order from the NEON intrinsics on big-endian: (1) brace initializers and (2) loads and stores through vector pointer casts.
(1)
uint32x4_t v = { 0, 1, 2, 3 };givesvgetq_lane_u32(v, 0) == 3, wherevld1q_u32of the same array gives 0.(2)
((uint8x16_t*)p)[0]putsp[15]in lane 0, wherevld1q_u8(p)putsp[0]there.On little-endian both orders agree, but on big-endian
volk_profileflagged 13 implementations:32f_index_min_16u,32fc_index_min_16uand32fc_index_max_16u32f_index_min_32u,32fc_index_min_32u,32fc_index_max_32u,32fc_conjugate_32fc,32u_byteswapand32u_reverse_32uconv_k7_r2Three more have the same bug but pass the lib/kernel_tests.h cases that
volk_profileandvolk_test_allrun:32f_index_max_16u(neon, neonv8) and32f_index_max_32u(neonv8). The test input starts with 16 tied maxima, and the kernel returns the smallest tied index, so reversed lane order is hidden. A similar test with a single maximum shows them returning the wrong index before the PR's changes and the right one after. I did not change the tests for this PR, they're fine for their intended purpose, they just don't catch the particular bugs I'm fixing here.The fixes:
conjugate: load the constant from astatic constarray withvld1qbyteswapandreverse_32u: replace the table lookup withvrev32q_u8, which needs no index vectorneonspiral: route its loads and stores through two macros, usingvld1q/vst1qwhen __ARM_BIG_ENDIAN is defined but keeping the pointer casts otherwise, so little-endian generated code for neonspiral is unchanged. (Took different paths here because on 32-bit ARM,vld1has no immediate-offset addressing, so switching would have added address arithmetic and register spills to the loop)Testing:
On aarch64_be hardware, as well as qemu-aarch64_be: full volk_profile ran with 0 failures (vs 13 on main).
On qemu-aarch64 and qemu-arm (armhf): full volk_profile plus the gtest suite, all passing, same as main