From e5eb60ef4b40510b28769f8e5b8b398350616e17 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Mon, 13 Apr 2026 09:58:21 +0200 Subject: [PATCH] Prefer the 128 bit multiplication over the assembler ARM64 implementation The asm code looks correct, but there is a report about a failure of the self test on a Apple Silicon, maybe caused by a compiler bug. Considering the 128 bit integer implementation is anyway equally fast, makes sense to always use it. See: https://github.com/amadvance/snapraid/issues/43 --- HISTORY | 8 ++++++++ cmdline/util.c | 45 ++++++++++++++++++++++++++++++++++++++------- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/HISTORY b/HISTORY index f82e98b6..377263ab 100644 --- a/HISTORY +++ b/HISTORY @@ -1,6 +1,14 @@ SnapRAID HISTORY ================ +14.3 2026/04 +============ + * On ARM64 (AArch64), replaced the inline assembly implementation of + 128-bit multiplication with a uint128_t based version. + This avoids potential miscompilations observed with aggressive + compiler optimizations (e.g. -O2/-O3) on some toolchains (Apple + Silicon), while still generating optimal code on modern compilers. + 14.2 2026/04 ============ * Added a workaround for filesystems with a broken implementation of diff --git a/cmdline/util.c b/cmdline/util.c index 6da26f1d..2296ad89 100644 --- a/cmdline/util.c +++ b/cmdline/util.c @@ -562,6 +562,13 @@ static inline void util_write64(void* ptr, uint64_t v) * Overwrites B contents with C's high 64 bits. */ #if defined(__x86_64__) || defined(_M_X64) +/* + * This is intentionally placed before the __SIZEOF_INT128__ implementation. + * + * This implementation happens to generate more efficient code. + * While this likely depends only on the compiler, as long as it remains faster, + * we will keep it in this order. + */ static inline void util_mum(uint64_t* A, uint64_t* B) { uint64_t a = *A; @@ -578,7 +585,38 @@ static inline void util_mum(uint64_t* A, uint64_t* B) *A = low; *B = high; } +#elif defined(__SIZEOF_INT128__) +static inline void util_mum(uint64_t* A, uint64_t* B) +{ + __uint128_t product = ((__uint128_t)*A) * ((__uint128_t)*B); + *A = (uint64_t)product; + *B = (uint64_t)(product >> 64); +} #elif defined(__aarch64__) || defined(_M_ARM64) +/* + * This is intentionally placed after the __SIZEOF_INT128__ implementation. + * + * Although the code looks correct, there is a report of this + * implementation failing the self-test on macOS (Darwin 25.4.0, arm64) + * with Apple clang 21.0.0 (clang-2100.0.123.102). + * + * See: https://github.com/amadvance/snapraid/issues/43 + * + * Note that the implementation is the same of OpenSSL: + * + * #elif defined(__aarch64__) && defined(SIXTY_FOUR_BIT_LONG) + * #if defined(__GNUC__) && __GNUC__ >= 2 + * #define BN_UMULT_HIGH(a, b) ({ \ + * register BN_ULONG ret; \ + * asm ("umulh %0,%1,%2" \ + * : "=r"(ret) \ + * : "r"(a), "r"(b)); \ + * ret; }) + * #endif + * #endif + * + * See: https://github.com/openssl/openssl/blob/0463cbf185680e66a3a8960f10cd33001377d28e/crypto/bn/bn_local.h#L439 + */ static inline void util_mum(uint64_t* A, uint64_t* B) { uint64_t a = *A; @@ -593,13 +631,6 @@ static inline void util_mum(uint64_t* A, uint64_t* B) *A = low; *B = high; } -#elif defined(__SIZEOF_INT128__) -static inline void util_mum(uint64_t* A, uint64_t* B) -{ - __uint128_t product = ((__uint128_t)*A) * ((__uint128_t)*B); - *A = (uint64_t)product; - *B = (uint64_t)(product >> 64); -} #else static inline void util_mum(uint64_t* A, uint64_t* B) {