Closed mscdex closed 2 years ago
Apparently it only happens when compiling with something like -O0
.
So the problem is that the following set of four registers, which together form the lookup table, are not sequentially numbered:
{v5.16b, v6.16b, v7.16b, v16.16b}
That sucks, because as you mention, the code goes to great lengths to load that table into four hardcoded sequential registers: v8
, v9
, v10
and v11
.
For some unclear reason, the compiler chooses to rename those registers when returning from the function. I was really hoping that any reasonable compiler would never do that, because the hardcoded registers are already taken and the table stays live for the duration of the encoder.
Yet here we are. My little gambit failed.
Testing a fix sucks, because I don't have an ARM64 machine that I can test on, and even then I'm not sure that I can reproduce the bug.
The silver lining is that clang
should not be affected by the codegen bug that GCC has for vld1q_u8_x4
. So we should hopefully be able to use that instead...
Could you try changing line 28 to this:
#if defined(BASE64_NEON64_USE_ASM) && !defined(__clang__)
Another thing to try is to add the always_inline
attribute to the function:
__attribute__((always_inline))
static inline uint8x16x4_t
load_64byte_table (const uint8_t *p)
{
#ifdef BASE64_NEON64_USE_ASM
I believe that -O0
can turn off inlining, and that may mean that the compiler can't make the reasonable inference that it should not rename the registers.
Both suggestions result in the same compiler errors.
FWIW I don't have an arm64 device handy either, so I just installed and used clang (v14) with an aarch64 sysroot (https://developer.arm.com/-/media/Files/downloads/gnu-a/10.3-2021.07/binrel/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu.tar.xz).
Here's the command line I'm using (from the project root) to test FWIW (on Linux):
clang-14 -DHAVE_NEON64=1 -I./include -I./lib -O0 -I/tmp/aarch64-none-linux-gnu/libc/usr/include -target arm64-linux-gnu -c lib/arch/neon64/codec.c -o base64_neon64.codec.o
Thanks for linking to the sysroot and for sharing your script! Those will be useful in the future. I was able to reproduce the bug and also affirm your conclusions that my proposed fixes don't work.
This looks like a nasty bug. Even when I inline the table-loading code into the encoder loop, the bug appears. Even when I don't create a uint8x16x4_t
, but pass the t0-t3
registers (which should surely be in v8-v11
...) directly to the inline assembly, the bug manifests itself.
I'm unsure of how to fix this, other than to rewrite the whole encoder logic in assembly. (That was something that I was actually planning on, because it would let me interleave loads and stores more naturally.)
Maybe the best fix for the time being is indeed the one you pushed: to just disable inline asm for clang when not optimizing.
Yesterday I set up a small AArch64 Debian VM using qemu-system-aarch64
to do quick prototyping on the AArch64 platform. I was hoping that it would be relatively simple to rewrite the entire NEON64 encoding loop in inline assembly, and it turns out I was right. AArch64 assembly is pretty approachable. I managed to implement the entire loop in inline assembly, including proper interleaving and pipelining of the 8x unrolled loop. All tests pass, and I'm reasonably happy with the cleanness of the code.
I've created a new issue (#98) for this enhancement and also pushed a testing branch, issue98
.
This was the nuclear option, but also the only solution I saw to fixing this bug. I was not hopeful that I could find any more tricks to get the compiler to generate the correct code by itself.
clang must not be allocating
l3
in a contiguous register? While building 3eab8e6ca57f4514dea4fd0ec435967e96371bbe, the compiler errors are: