Skip to content

fix(arm64): FMA-safe float-ADM DWT2 bit-exactness — guard scalar adm_dwt2_s (closes T-NEON-FMA, ADR-1057) - #1060

Merged
lusoris merged 1 commit into
masterfrom
fix/neon-fma-float-adm-dwt2
Jun 27, 2026
Merged

fix(arm64): FMA-safe float-ADM DWT2 bit-exactness — guard scalar adm_dwt2_s (closes T-NEON-FMA, ADR-1057)#1060
lusoris merged 1 commit into
masterfrom
fix/neon-fma-float-adm-dwt2

Conversation

@lusoris

@lusoris lusoris commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

FMA-safe float-ADM DWT2 bit-exactness — closes T-NEON-FMA-FLOAT-ADM-DWT2 (ADR-1057)

The backlog item was still open (stale state said otherwise). Root cause (subtle): the NEON DWT2 kernel was already FMA-free + dispatched, but the scalar reference adm_dwt2_s/adm_dwt2_lo_s in adm_tools.c compiled with the default -ffp-contract=fast, so on aarch64 it fused a*b+c → fmadd and diverged 1-ULP from the FMA-free NEON path (the opposite of ADR-1057's assumption). Also adm_tools.c's #include "config.h" was behind a dead #ifdef HAVE_CONFIG_H, so ARCH_AARCH64 was never visible there.

Fix: include config.h unconditionally; aarch64-only -ffp-contract=off on the two scalar DWT2 functions (x86 byte-identical). Adds the missing load-bearing test_float_adm_simd.c (NEON == scalar, 4 bands × 9 fixtures incl. tail + odd dims).

Verification: real meson aarch64 cross-build under qemu-aarch64 → 9/9 pass with fix, 9/9 fail without (load-bearing). x86 meson test --suite=fast 104/104; test_float_adm_coverage confirms scalar ADM byte-unchanged → Netflix golden untouched.

Deep-dive deliverables (ADR-0108)

  • Research digest — no digest needed: updated the existing docs/research/arm64-neon-bit-exactness-audit-2026-05-30.md (corrected its dead-code claim); no new digest warranted.
  • Decision matrix — ADR-1057 ## Alternatives considered (corrected FMA-contract diagnosis + options).
  • AGENTS.md invariant note — core/src/feature/arm64/AGENTS.md (the -ffp-contract=off FMA invariant).
  • Reproducer / smoke-test command — meson test -C build-arm64 --suite=fast test_float_adm_simd under qemu-aarch64 (RED without the guard).
  • CHANGELOG fragment — changelog.d/fixed/neon-fma-safe-float-adm-dwt2-bitexact.md.
  • Rebase note — docs/rebase-notes.md (FMA-contract invariant: the scalar DWT2 must stay -ffp-contract=off on aarch64).

docs/state.md Open→Recently-closed (T-NEON-FMA-FLOAT-ADM-DWT2). no docs needed: ARM bit-exactness bug fix, no user-discoverable surface change.

🤖 Generated with Claude Code

@lusoris @claude
fix(arm64): FMA-safe float-ADM DWT2 bit-exactness — guard scalar adm_…
0e1c5a4 
…dwt2_s (ADR-1057)

T-NEON-FMA-FLOAT-ADM-DWT2 was still open: the NEON DWT2 kernel was correctly
FMA-free + dispatched, but the SCALAR reference adm_dwt2_s/adm_dwt2_lo_s in
adm_tools.c compiled with the default -ffp-contract=fast, so on aarch64 it
fused a*b+c into fmadd and diverged 1-ULP from the FMA-free NEON path. Fix:
include config.h unconditionally (ARCH_AARCH64 was never visible — dead
HAVE_CONFIG_H guard) + aarch64-only -ffp-contract=off on the two scalar
DWT2 fns; x86 byte-identical. Adds the missing load-bearing
test_float_adm_simd.c (NEON==scalar bit-exact). qemu-aarch64 cross-build:
9/9 pass with fix, 9/9 fail without; x86 fast suite 104/104; golden untouched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@lusoris
lusoris marked this pull request as ready for review June 27, 2026 18:18
@lusoris
lusoris force-pushed the fix/neon-fma-float-adm-dwt2 branch from 2367ec5 to 0e1c5a4 Compare June 27, 2026 18:18
@lusoris
lusoris merged commit 2d2f452 into master Jun 27, 2026
@lusoris
lusoris deleted the fix/neon-fma-float-adm-dwt2 branch June 27, 2026 18:18
lusoris added a commit that referenced this pull request Jun 27, 2026
@lusoris @claude
fix(go,rust): bound GPU/AI probe subprocesses + Rust -sys picture dou…
8c41e90 
…ble-free (gorust-rederive)

Re-derives the go-rust-build bug-hunt sweep cleanly onto current master.
The original fix/bughunt-go-rust branch is a corrupted orphan whose
owned-path delta would revert merged work, so each fix was re-implemented
from intent and verified against master (guess + check).

1. pkg/gpu/detect.go::runProbe set cmd.WaitDelay but never gave the command
   a context. WaitDelay alone cannot cap a child that produces no output, so
   a wedged nvidia-smi / driver-blocked rocm-smi could stall gpu.Detect() and
   node startup indefinitely. Switched to exec.CommandContext +
   context.WithTimeout(probeTimeout) plus a 1s grace WaitDelay; the timeout
   now fires and Detect() falls back to CPU.

2. pkg/ai/infer.go::Registry.Infer ran vmafx-ort-runner via exec.Command (no
   context). Added a ctx context.Context first parameter + exec.CommandContext
   with an inferTimeout upper bound when the caller supplies no deadline
   (mirrors pkg/encoder / pkg/bisect); 3 test call-sites updated.

3. bindings/rust/vmafx-sys/src/safe.rs::read_pictures borrowed &mut VmafPicture
   while keeping unref_picture public — a post-transfer double-free footgun.
   Now consumes both pictures by value (use-after-move is a compile error).
   The error path does NOT manually unref: the libvmaf contract takes ownership
   for the call's duration, so a second unref is a use-after-free against a
   CUDA-enabled libvmaf. This aligns the -sys crate with the vmafx crate's
   Context::read_pictures contract settled by PR #1056 (round-3 R3-2).
   DEDUP: the vmafx crate's separate read_pictures double-free was ALREADY
   fixed by PR #1056 (manual unref dropped) and is not re-touched here.

4. core/src/meson.build comment claimed enable_rust_features defaults true;
   core/meson_options.txt sets value: false. Corrected to the single source
   of truth.

Also restores two docs that were accidentally truncated to 0 bytes on master
by unrelated PRs: docs/state.md (wiped by #1055, pelorus ABI re-vendor) and
docs/rebase-notes.md (wiped by #1060, FMA-ADM fix). Both are recovered from
their last-good blobs; the large insertion count in this diff is recovered
data, not new content. The deliverable rows/entries for this change are added
on top of the restored files.

No Netflix golden assertions touched; all fixes are off the metric path.

Go: go build ./... clean; go vet/test pkg/gpu + pkg/ai + cmd/vmafx-controller
pass; gofmt clean. Rust: cargo build/test/clippy on vmafx-sys green
(netflix_golden_score integration test passes by-move); vmafx crate build+test
still green incl. #1056's UAF smoke test; cargo fmt clean on touched files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 27, 2026
@lusoris @claude
fix(go,rust): bound GPU/AI probe subprocesses + Rust -sys picture dou…
f3bab90 
…ble-free (gorust-rederive) (#1052)

Re-derives the go-rust-build bug-hunt sweep cleanly onto current master.
The original fix/bughunt-go-rust branch is a corrupted orphan whose
owned-path delta would revert merged work, so each fix was re-implemented
from intent and verified against master (guess + check).

1. pkg/gpu/detect.go::runProbe set cmd.WaitDelay but never gave the command
   a context. WaitDelay alone cannot cap a child that produces no output, so
   a wedged nvidia-smi / driver-blocked rocm-smi could stall gpu.Detect() and
   node startup indefinitely. Switched to exec.CommandContext +
   context.WithTimeout(probeTimeout) plus a 1s grace WaitDelay; the timeout
   now fires and Detect() falls back to CPU.

2. pkg/ai/infer.go::Registry.Infer ran vmafx-ort-runner via exec.Command (no
   context). Added a ctx context.Context first parameter + exec.CommandContext
   with an inferTimeout upper bound when the caller supplies no deadline
   (mirrors pkg/encoder / pkg/bisect); 3 test call-sites updated.

3. bindings/rust/vmafx-sys/src/safe.rs::read_pictures borrowed &mut VmafPicture
   while keeping unref_picture public — a post-transfer double-free footgun.
   Now consumes both pictures by value (use-after-move is a compile error).
   The error path does NOT manually unref: the libvmaf contract takes ownership
   for the call's duration, so a second unref is a use-after-free against a
   CUDA-enabled libvmaf. This aligns the -sys crate with the vmafx crate's
   Context::read_pictures contract settled by PR #1056 (round-3 R3-2).
   DEDUP: the vmafx crate's separate read_pictures double-free was ALREADY
   fixed by PR #1056 (manual unref dropped) and is not re-touched here.

4. core/src/meson.build comment claimed enable_rust_features defaults true;
   core/meson_options.txt sets value: false. Corrected to the single source
   of truth.

Also restores two docs that were accidentally truncated to 0 bytes on master
by unrelated PRs: docs/state.md (wiped by #1055, pelorus ABI re-vendor) and
docs/rebase-notes.md (wiped by #1060, FMA-ADM fix). Both are recovered from
their last-good blobs; the large insertion count in this diff is recovered
data, not new content. The deliverable rows/entries for this change are added
on top of the restored files.

No Netflix golden assertions touched; all fixes are off the metric path.

Go: go build ./... clean; go vet/test pkg/gpu + pkg/ai + cmd/vmafx-controller
pass; gofmt clean. Rust: cargo build/test/clippy on vmafx-sys green
(netflix_golden_score integration test passes by-move); vmafx crate build+test
still green incl. #1056's UAF smoke test; cargo fmt clean on touched files.

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@lusoris lusoris mentioned this pull request Jun 27, 2026
Merged
6 of 9 tasks
lusoris added a commit that referenced this pull request Jun 27, 2026
@lusoris @claude
fix(ci): repair master red — TSan operator-new dup + revert #1060 ARM…
edbc0f1 
… golden drift

Two regressions rode into master via the 2026-06-27 admin-merge batch (per-PR
CI was bypassed). Both are required-check failures.

1. Sanitizers (TSan) link failure. The R2-9 OOM-injection test
   `test_gpu_dispatch_env_oom.cpp` replaces the global `operator new` /
   `operator delete`; under TSan/ASan/MSan the sanitizer runtimes interpose
   their own, so lld reports `duplicate symbol: operator new(unsigned long)`.
   Guard the override and self-skip the test under
   `__SANITIZE_THREAD__`/`__SANITIZE_ADDRESS__`/`__SANITIZE_MEMORY__` and the
   Clang `__has_feature` equivalents. The R2-9 slot-poisoning check still runs
   in every non-sanitized suite, so coverage is retained.

2. ARM golden drift. PR #1060's aarch64-only `-ffp-contract=off` guard on the
   scalar `adm_dwt2_s` / `adm_dwt2_lo_s` shifted the akiyo `disable_enhn_gain`
   ADM score on the ARM build matrix (88.030463 -> 88.030322), failing the
   `vmafexec_test.py` golden assertions. x86 D24 was unaffected (the guard was
   `ARCH_AARCH64`-gated), which is why it was not caught pre-merge. Per global
   rule #1 (immutable golden; fix the code, never the assertion) the scalar
   guard is reverted: `adm_tools.c` returns to its pre-#1060 FMA-default scalar
   arithmetic on aarch64 (byte-identical to 2d2f452^), restoring 88.030463.
   The now-stale `test_float_adm_simd.c` parity test and its meson
   registration are removed. The NEON-vs-scalar parity gap returns to the
   undispatched/FMA-free state of ADR-1057; T-NEON-FMA-FLOAT-ADM-DWT2-2026-06-06
   stays Open for a correct re-attempt (make the NEON side match the scalar FMA
   reference; validate against the full ARM quality suite, not only x86 golden).

Docs: ADR-1057 Update (2026-06-27) records the reverted follow-up and the
lesson; docs/state.md adds the Recently-closed row (and fixes a stale ADR-1057
link); changelog fragment replaced; arm64 AGENTS.md + research digest restored.

Verified: CPU build OK; fast suite 105/105; `test_gpu_dispatch_env_oom` builds
and passes (override active in the non-sanitized config);
`git diff 2d2f452^ -- core/src/feature/adm_tools.c` empty; clang-format,
assertion-density, check-copyright clean on touched files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 27, 2026
@lusoris @claude
fix(ci): repair master red — TSan operator-new dup + revert #1060 ARM…
27f5a25 
… golden drift (#1063)

* fix(ci): repair master red — TSan operator-new dup + revert #1060 ARM golden drift

Two regressions rode into master via the 2026-06-27 admin-merge batch (per-PR
CI was bypassed). Both are required-check failures.

1. Sanitizers (TSan) link failure. The R2-9 OOM-injection test
   `test_gpu_dispatch_env_oom.cpp` replaces the global `operator new` /
   `operator delete`; under TSan/ASan/MSan the sanitizer runtimes interpose
   their own, so lld reports `duplicate symbol: operator new(unsigned long)`.
   Guard the override and self-skip the test under
   `__SANITIZE_THREAD__`/`__SANITIZE_ADDRESS__`/`__SANITIZE_MEMORY__` and the
   Clang `__has_feature` equivalents. The R2-9 slot-poisoning check still runs
   in every non-sanitized suite, so coverage is retained.

2. ARM golden drift. PR #1060's aarch64-only `-ffp-contract=off` guard on the
   scalar `adm_dwt2_s` / `adm_dwt2_lo_s` shifted the akiyo `disable_enhn_gain`
   ADM score on the ARM build matrix (88.030463 -> 88.030322), failing the
   `vmafexec_test.py` golden assertions. x86 D24 was unaffected (the guard was
   `ARCH_AARCH64`-gated), which is why it was not caught pre-merge. Per global
   rule #1 (immutable golden; fix the code, never the assertion) the scalar
   guard is reverted: `adm_tools.c` returns to its pre-#1060 FMA-default scalar
   arithmetic on aarch64 (byte-identical to 2d2f452^), restoring 88.030463.
   The now-stale `test_float_adm_simd.c` parity test and its meson
   registration are removed. The NEON-vs-scalar parity gap returns to the
   undispatched/FMA-free state of ADR-1057; T-NEON-FMA-FLOAT-ADM-DWT2-2026-06-06
   stays Open for a correct re-attempt (make the NEON side match the scalar FMA
   reference; validate against the full ARM quality suite, not only x86 golden).

Docs: ADR-1057 Update (2026-06-27) records the reverted follow-up and the
lesson; docs/state.md adds the Recently-closed row (and fixes a stale ADR-1057
link); changelog fragment replaced; arm64 AGENTS.md + research digest restored.

Verified: CPU build OK; fast suite 105/105; `test_gpu_dispatch_env_oom` builds
and passes (override active in the non-sanitized config);
`git diff 2d2f452^ -- core/src/feature/adm_tools.c` empty; clang-format,
assertion-density, check-copyright clean on touched files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(ci): Windows MSVC+CUDA D8021 — filter -Wno-write-strings via get_supported_arguments

The third required-check failure on master (pre-existing): the
`test_gpu_dispatch_env_oom` meson target passed the raw GCC/Clang flag
`-Wno-write-strings` unconditionally. MSVC `cl` parses `/W` as a warning-level
prefix, so it rejects the flag with
`D8021: invalid numeric argument '/Wno-write-strings'`, failing
`Build — Windows MSVC + CUDA (build only)`. Route the flag through
`cxx.get_supported_arguments('-Wno-write-strings')` so MSVC drops it while
GCC/Clang keep it (the flag is only needed to silence -Wwrite-strings from the
shared upstream `mu_assert` macro on the C++ TU).

A full grep confirms `-Wno-write-strings` was the only raw `-Wno-*` reaching the
compiler directly; `-ffp-contract=off` elsewhere is only a D9002 warning on MSVC
(ignored), and the lone `-Wl,-framework` is macOS-gated.

Verified: CPU build OK; fast suite 105/105; the flag still applies on Linux clang
(get_supported_arguments returns it). Completes the master-CI repair started in
this PR (Sanitizers-thread + D24 golden already green on the PR run).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(ci): Windows MSVC C2440 — hold mu_assert messages in static char[] (supersede flag hack)

Follow-up to the get_supported_arguments(-Wno-write-strings) change: dropping the
flag on MSVC merely exposed the real, hard error it was masking —

  test_gpu_dispatch_env_oom.cpp(145): error C2440: 'return': cannot convert
  from 'const char [70]' to 'char *'

test.h's `mu_assert` does `return message;`. The harness is C-first, where a
string literal -> `char *` is legal; this is the ONLY test whose body is a C++
TU using mu_assert, and under MSVC /std:c++latest that conversion is a hard
C2440 (no flag silences an error). The other `.cpp` test executables either
build from `.c` bodies (test_dict/model/feature) or pull in library `.cpp`
sources with no mu_assert, so they are unaffected — this file is uniquely
exposed.

Fix: hold the two assert messages in `static char[]` buffers. A mutable array
decays to `char *` with no conversion (no C2440, no -Wwrite-strings), and static
storage keeps the returned pointer valid after the function returns. The
obsolete `-Wno-write-strings` cpp_args is removed. Compiles identically on
MSVC / GCC / Clang.

Verified: CPU build OK; fast suite green; test_gpu_dispatch_env_oom builds +
passes; clang-format clean. Completes the master-CI repair (all three required
failures: TSan dup-symbol, ARM golden drift, Windows MSVC C2440).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@lusoris lusoris mentioned this pull request Aug 30, 2026
Merged
4 of 7 tasks
lusoris added a commit that referenced this pull request Aug 30, 2026
@lusoris @claude
fix(arm64): adm_dwt2_8_neon dropped the 4th filter tap at j=0 (ADR-10…
a013c14 
…57) (#1134)

The aarch64 Netflix golden failure (akiyo 88.030322 vs 88.030463, delta
1.41e-04, three failing assertions on Build - Ubuntu ARM clang (CPU)) is an
integer off-by-one, not the FMA-contraction problem ADR-1057 describes.

adm_dwt2_8_neon special-cases the j = 0 output column because its source indices
are mirrored ({1, 0, 1, 2} rather than the regular 2j-1..2j+2 stride). That
special case accumulated with

    for (int idx = 0; idx < 3; idx++)

while the scalar reference adm_dwt2_8 in core/src/feature/integer_adm.c
multiplies all four taps (filter_lo[0..3] against s0..s3 from ind_x[0..3][j]).
The NEON path therefore dropped ind_x[3][0] and its coefficient -4240 from the
first output column of every DWT2 row, on every scale. Fix: idx < 4.

Why ADR-1057's diagnosis was wrong: integer ADM accumulates in int64, so FP
contraction cannot affect it. Building the entire tree with -ffp-contract=off on
aarch64 leaves the score at 88.030322, byte-for-byte unchanged. That is why
PR #1060's fp-contract approach could not have worked and #1063 reverted it.

Reproduced and fixed locally without a CI round-trip, using an
aarch64-linux-gnu-gcc cross-build under qemu-aarch64-static:

  NEON                     integer_adm2=1.116698 adm3=1.052571 scale3=1.13728
  -Denable_asm=false       integer_adm2=1.116701 adm3=1.052573 scale3=1.13729
  NEON + -ffp-contract=off 88.030322  (unchanged - rules out FP contraction)
  after this fix           38 passed, 1 skipped, 0 failed on aarch64
                           (vmafexec_test.py + result_test.py, all akiyo
                           golden assertions included)

x86 is unaffected: the change is confined to core/src/feature/arm64/adm_neon.c.

No NEON-vs-scalar parity test covers the ADM DWT2 kernels on any architecture -
test_integer_adm_simd.c only exercises adm_cm_avx2 - which is how a three-vs-four
tap error shipped. Only the ARM golden leg caught it, and that leg is not a
required check, so it stayed red. Both gaps are worth closing separately.

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lusoris lusoris added this to the 1.0.0 — First release milestone Sep 4, 2026
@lusoris lusoris mentioned this pull request Sep 5, 2026
Merged
5 of 9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0 — First release

Development

Successfully merging this pull request may close these issues.

1 participant

@lusoris