chunkers: buzhash64 blockwise hit test, clang scalar-loop pins, always_inline (kernel codegen fixes)

docs/changes.rst

@@ -216,6 +216,8 @@ Other changes:
 - setuptools-scm: drop tag.strict, #10193
 - diff: show timestamp changes with full nanosecond precision, #9147
 - benchmark cpu: also benchmark zstd,-4
+- buzhash64: faster blockwise kernel (the hit test as a short-circuit chain) and make
+  it the aarch64 default, +24% over the NEON kernel on an Apple M3 Pro
 - archive: resolve the item metadata stream chunk ids lazily, big win for repo-list
   on remote repos, #10204
 - lock exceptions: tell who holds the lock, #2261

docs/usage/general/environment.rst.inc

@@ -246,10 +246,12 @@ General:
     BORG_FASTCDC_KERNEL / BORG_BUZHASH64_KERNEL
         Select the scan kernel the ``fastcdc`` / ``buzhash64`` chunker uses. Accepted values
         are ``avx512``, ``avx2``, ``neon``, ``blockwise`` and ``scalar``.
-        The default is whichever benchmarked fastest for the architecture: ``neon`` on
-        aarch64, and ``scalar`` (the plain sequential loop) on x86-64, where the compiler
-        folds the rolling hash update into a single instruction and thereby beats the vector
-        kernels. Other architectures get ``blockwise``, the portable multi-lane C kernel.
+        The default is whichever benchmarked fastest for the architecture: ``scalar`` (the
+        plain sequential loop) on x86-64, where the compiler folds the rolling hash update
+        into one or two instructions and thereby beats the vector kernels; on aarch64
+        ``neon`` for ``fastcdc`` and ``blockwise`` for ``buzhash64``, whose NEON kernel
+        loses to the portable multi-lane C kernel there. Other architectures get
+        ``blockwise``.
         All kernels chunk identically - same cut points, same chunk ids - and differ only in
         speed, so this is safe to change at any time, also for an existing repository.
         Which kernel is fastest is not predictable from the instruction set: it depends on the

src/borg/archiver/help_cmd.py

@@ -840,10 +840,12 @@ class HelpMixIn:
             BORG_FASTCDC_KERNEL / BORG_BUZHASH64_KERNEL
                 Select the scan kernel the ``fastcdc`` / ``buzhash64`` chunker uses. Accepted values
                 are ``avx512``, ``avx2``, ``neon``, ``blockwise`` and ``scalar``.
-                The default is whichever benchmarked fastest for the architecture: ``neon`` on
-                aarch64, and ``scalar`` (the plain sequential loop) on x86-64, where the compiler
-                folds the rolling hash update into a single instruction and thereby beats the vector
-                kernels. Other architectures get ``blockwise``, the portable multi-lane C kernel.
+                The default is whichever benchmarked fastest for the architecture: ``scalar`` (the
+                plain sequential loop) on x86-64, where the compiler folds the rolling hash update
+                into one or two instructions and thereby beats the vector kernels; on aarch64
+                ``neon`` for ``fastcdc`` and ``blockwise`` for ``buzhash64``, whose NEON kernel
+                loses to the portable multi-lane C kernel there. Other architectures get
+                ``blockwise``.
                 All kernels chunk identically - same cut points, same chunk ids - and differ only in
                 speed, so this is safe to change at any time, also for an existing repository.
                 Which kernel is fastest is not predictable from the instruction set: it depends on the

src/borg/chunkers/buzhash64_impl.c

@@ -21,9 +21,10 @@
  * Trot[b] = ROTL(T[b], window_size % 64) is precomputed by the caller, which
  * also removes one rotate per byte from the sequential path.
  *
- * Kernel dispatch: AVX-512 or AVX2 on x86-64 (runtime-detected), blockwise
- * scalar everywhere else, including aarch64 by default - its NEON kernel is
- * selectable by name but not auto-selected, see the note above it.
+ * Which kernel runs is decided per platform by bz64_kernel_default(): the
+ * sequential loop on x86-64, blockwise everywhere else, including aarch64,
+ * where the NEON kernel is selectable by name but not auto-selected (see the
+ * note above it); BORG_BUZHASH64_KERNEL overrides that.
  * All kernels return bit-identical results. */
 
 #include <stdlib.h>
@@ -33,6 +34,25 @@
 
 #define BZ_ROTL(x, k) (((x) << ((k) & 63)) | ((x) >> ((64 - (k)) & 63)))
 
+/* Pin a value: the compiler must materialize it here and cannot re-associate
+ * expressions across it (an empty asm statement that "modifies" it). Only
+ * clang needs it (see bz64_scan_seq); gcc emits the wanted form by itself and
+ * is left alone, an asm statement can move its code around. */
+#if defined(__clang__)
+#define BZ_PIN(v) __asm__("" : "+r"(v))
+#else
+#define BZ_PIN(v) ((void)0)
+#endif
+
+/* The per-block table lookup helpers must be inlined into the scan loops:
+ * gcc at -O2 (Debian's python flags) leaves them as calls otherwise, one per
+ * 8-byte block, and passes their 64-byte result through the stack. */
+#if defined(__GNUC__)
+#define BZ_ALWAYS_INLINE static inline __attribute__((always_inline))
+#else
+#define BZ_ALWAYS_INLINE static inline
+#endif
+
 /* --- sequential reference loop (also: block re-scan and tail) ----------- */
 
 static size_t bz64_scan_seq(const uint64_t *T, const uint64_t *Trot,
@@ -42,7 +62,14 @@ static size_t bz64_scan_seq(const uint64_t *T, const uint64_t *Trot,
     uint64_t sum = *sum_io;
     size_t j = 0;
     while (j < n && (sum & mask) != 0) {
-        sum = BZ_ROTL(sum, 1) ^ Trot[pr[j]] ^ T[pa[j]];
+        /* The two table values are combined off the chain and the result is
+         * pinned, so that the loop-carried dependency is exactly rotate + one
+         * xor (2 cycles per byte). Unpinned, clang re-associates the xors into
+         * the chain and makes it 3 cycles per byte - M3 Pro: 1210 vs 1780 MB/s
+         * of scanned data; gcc emits the 2-cycle form either way. */
+        uint64_t d = Trot[pr[j]] ^ T[pa[j]];
+        BZ_PIN(d);
+        sum = BZ_ROTL(sum, 1) ^ d;
         j++;
     }
     *sum_io = sum;
@@ -52,7 +79,7 @@ static size_t bz64_scan_seq(const uint64_t *T, const uint64_t *Trot,
 /* Load the block's 8 out/in byte pairs (via two 8-byte data loads) and
  * compute the aligned-domain prefix XORs s[0..7] with a depth-3 tree.
  * Endianness-independent: bytes are extracted by shifting. */
-static inline void bz64_block_prefix(const uint64_t *T, const uint64_t *Trot,
+BZ_ALWAYS_INLINE void bz64_block_prefix(const uint64_t *T, const uint64_t *Trot,
                                      const uint8_t *pr, const uint8_t *pa, uint64_t s[8])
 {
     uint64_t wr, wa;
@@ -82,7 +109,7 @@ static inline void bz64_block_prefix(const uint64_t *T, const uint64_t *Trot,
     s[7] = s8;
 }
 
-/* --- blockwise scalar (the default kernel) --------------------------------- */
+/* --- blockwise scalar (the default kernel, except on x86-64) --------------- */
 
 static size_t bz64_scan_blockwise(const uint64_t *T, const uint64_t *Trot,
                                 const uint8_t *pr, const uint8_t *pa,
@@ -97,10 +124,16 @@ static size_t bz64_scan_blockwise(const uint64_t *T, const uint64_t *Trot,
     while (j + 8 <= n && (sum & mask) != 0) {
         bz64_block_prefix(T, Trot, pr + j, pa + j, s);
         uint64_t c = BZ_ROTL(sum, 8);
-        uint64_t hit = 0;
-        for (int k = 0; k < 8; k++)
-            hit |= (((c ^ s[k]) & M[k]) == 0);
-        if (hit) {
+        /* The hit test is a short-circuit chain on purpose: it compiles to
+         * eight test-and-branch pairs that leave the loop as soon as a lane
+         * hits. Written as a loop accumulating `hit |= ...`, clang lowers it
+         * into a chain of conditional increments (M3 Pro: 20% slower) and
+         * gcc >= 12 at -O2 vectorises it, reloading the eight 8-byte stores
+         * of s[] as 16-byte vectors - which fails store-to-load forwarding
+         * on every block (Zen 4: 2.2x slower). */
+        if (((c ^ s[0]) & M[0]) == 0 || ((c ^ s[1]) & M[1]) == 0 || ((c ^ s[2]) & M[2]) == 0 ||
+            ((c ^ s[3]) & M[3]) == 0 || ((c ^ s[4]) & M[4]) == 0 || ((c ^ s[5]) & M[5]) == 0 ||
+            ((c ^ s[6]) & M[6]) == 0 || ((c ^ s[7]) & M[7]) == 0) {
             size_t r = bz64_scan_seq(T, Trot, pr + j, pa + j, 8, &sum, mask); /* exact re-scan */
             *sum_io = sum;
             return j + r;
@@ -116,28 +149,26 @@ static size_t bz64_scan_blockwise(const uint64_t *T, const uint64_t *Trot,
 
 /* --- NEON (aarch64) ------------------------------------------------------
  *
- * Nothing selects this by default (the default is the sequential kernel
- * everywhere); BORG_BUZHASH64_KERNEL=neon selects it. On an Apple M3 Pro it
- * loses to the blockwise kernel - 2590 vs 2360 MB/s, the same ~9% gap at
- * every mask size from 17 to 23 bits - so it is not the one to reach for
- * there.
+ * Nothing selects this by default; BORG_BUZHASH64_KERNEL=neon does. On an
+ * Apple M3 Pro it loses to the blockwise kernel: 2370 vs 2950 MB/s of
+ * scanned data at the kernel level (Apple clang 17, -O3, same at every mask
+ * size from 19 to 23 bits).
  *
  * Why it loses there: the 16 table lookups per block have to happen in
  * general registers (NEON has no gather), so the vector form only ADDS the
  * move to the SIMD side plus a cross-lane reduce (umaxv) before the loop
- * branch can resolve - all it saves is the 8-lane test, three cheap ops per
- * lane, which Apple's very wide scalar ALUs retire at more than one lane per
- * cycle anyway. fastcdc keeps NEON as its default because its per-lane test
+ * branch can resolve - all it saves is the 8-lane test, which as a
+ * short-circuit chain of test-and-branch pairs is cheap on a wide core and
+ * exits early. fastcdc keeps NEON as its default because its per-lane test
  * work is larger (add plus per-lane shifted masks), enough to pay for the
  * trip; there it wins by 2x.
  *
  * It is kept because that reasoning is about core width, and the measurement
  * comes from the widest scalar ARM core there is. Neoverse (Graviton,
  * Ampere), Cortex-A7x and friends are 3-4 wide on the scalar side with
- * comparatively healthy NEON, which is exactly where this should get
- * competitive - on this machine's much narrower E-cores the 9% gap already
- * collapses into measurement noise. If you have such hardware, compare
- * BORG_BUZHASH64_KERNEL=neon against =blockwise and please report. */
+ * comparatively healthy NEON, which is where this might get competitive. If
+ * you have such hardware, compare BORG_BUZHASH64_KERNEL=neon against
+ * =blockwise and please report. */
 
 #if defined(__aarch64__)
 #define BZ_KIND "neon"
@@ -190,7 +221,7 @@ static size_t bz64_scan_simd(const uint64_t *T, const uint64_t *Trot,
 /* Load the block's 8 per-byte deltas D_t = Trot[out_t] ^ T[in_t] (via two
  * 8-byte data loads), without the rotations and the prefix XOR: the vector
  * kernels do those in the vector domain. Endianness-independent. */
-static inline void bz64_block_delta(const uint64_t *T, const uint64_t *Trot,
+BZ_ALWAYS_INLINE void bz64_block_delta(const uint64_t *T, const uint64_t *Trot,
                                     const uint8_t *pr, const uint8_t *pa, uint64_t d[8])
 {
     uint64_t wr, wa;
@@ -409,16 +440,15 @@ int bz64_kernel_select(const char *name, int *out_id)
 
 int bz64_kernel_default(void)
 {
-#if defined(__aarch64__)
-    /* Same picture as for fastcdc: NEON is baseline on aarch64 and wins, so
-     * this only falls back if the build lacks it. */
-    int kid;
-    if (bz64_kernel_select("neon", &kid) == BZ_KSEL_OK)
-        return kid;
-    return BZ_K_BLOCKWISE;
-#elif defined(__x86_64__) || defined(_M_X64)
+#if defined(__x86_64__) || defined(_M_X64)
+    /* The compiler keeps the sequential loop's update at rol + xor, a 2-cycle
+     * chain that the block kernels' 16 table lookups per 8 bytes do not beat
+     * (Zen 4, gcc -O2: scalar 2.1 cycles/byte, blockwise 2.95, avx2/avx512
+     * 3.9 - see the double-buffer note above). */
     return BZ_K_SCALAR;
 #else
+    /* Everywhere else, including aarch64: unlike fastcdc's, this NEON kernel
+     * loses to the blockwise one (see the note above it), so blockwise it is. */
     return BZ_K_BLOCKWISE;
 #endif
 }

src/borg/chunkers/fastcdc_impl.c

@@ -30,13 +30,30 @@
 
 #include "fastcdc_impl.h"
 
+/* Pin a value: the compiler must materialize it here and cannot re-associate
+ * expressions across it (an empty asm statement that "modifies" it). Only
+ * clang needs it (see fc_scan_seq); gcc emits the wanted form by itself and
+ * is left alone, an asm statement can move its code around. */
+#if defined(__clang__)
+#define FC_PIN(v) __asm__("" : "+r"(v))
+#else
+#define FC_PIN(v) ((void)0)
+#endif
+
 /* --- sequential reference loop (also: block recheck and tail) ----------- */
 
 static int64_t fc_scan_seq(const uint64_t *gear, const uint8_t *p, size_t n, uint64_t *fp_io, uint64_t mask)
 {
     uint64_t fp = *fp_io;
     for (size_t i = 0; i < n; i++) {
-        fp = (fp << 1) + gear[p[i]];
+        /* The table value is pinned so that the update stays one instruction
+         * on the loop-carried chain: on x86-64 a lea (table value + fp * 2),
+         * 1 cycle per byte, which gcc emits anyway but clang only with the
+         * pin (unpinned it emits add + add-from-memory, 2 cycles per byte);
+         * on aarch64 it is one shifted add either way. */
+        uint64_t g = gear[p[i]];
+        FC_PIN(g);
+        fp = (fp << 1) + g;
         if ((fp & mask) == 0) {
             *fp_io = fp;
             return (int64_t)i;

src/borg/testsuite/chunkers/fastcdc_test.py

@@ -226,7 +226,8 @@ def test_fastcdc_portable_kernel_available(kernel, monkeypatch):
 # first: the first entry this build and CPU can actually run is the default.
 # Mirrors fc_kernel_default() / bz64_kernel_default() (fastcdc_impl.c,
 # buzhash64_impl.c) and phte_kernel_default() (phte_core.h).
-ROLLING_HASH_DEFAULTS = {"x86_64": ["scalar"], "aarch64": ["neon", "blockwise"]}
+FASTCDC_DEFAULTS = {"x86_64": ["scalar"], "aarch64": ["neon", "blockwise"]}
+BUZHASH64_DEFAULTS = {"x86_64": ["scalar"], "aarch64": ["blockwise"]}
 AES_DEFAULTS = {"x86_64": ["vaes", "aes-ni", "evp"], "aarch64": ["aes-arm64", "evp"]}
 
 
@@ -255,8 +256,10 @@ def expected_default_kernel(envvar, make, key, monkeypatch):
     machine = default_kernel_arch()
     if envvar == "BORG_AES_CHUNKER_KERNEL":
         preference = AES_DEFAULTS.get(machine, ["evp"])
+    elif envvar == "BORG_BUZHASH64_KERNEL":
+        preference = BUZHASH64_DEFAULTS.get(machine, ["blockwise"])
     else:
-        preference = ROLLING_HASH_DEFAULTS.get(machine, ["blockwise"])
+        preference = FASTCDC_DEFAULTS.get(machine, ["blockwise"])
     for kernel in preference:
         monkeypatch.setenv(envvar, kernel)
         try: