Skip to content

fix(simd): take the ssimulacra2 edge-diff difference in double, and gate ISA invariance - #1367

Closed
lusoris wants to merge 3 commits into
masterfrom
test/feature-isa-invariance
Closed

lusoris wants to merge 3 commits into
masterfrom
test/feature-isa-invariance

Conversation

@lusoris

@lusoris lusoris commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Follows #1363 (now merged). Rebased onto master; supersedes the auto-closed #1364.

The fork's contract is that a score does not depend on the host instruction set. Ten test_<feature>_simd.c files assert bit-exactness and all pass — yet ssimulacra2 scored differently with and without SIMD, twice (#1363/ADR-1205, and again here).

The structural reason is that those tests do not compare the two things that ship. The shipped scalar functions are static, so each test defines its own scalar reference and compares the kernel against that. It asserts kernel == test's idea of the reference when the property that matters is shipped SIMD path == shipped scalar path. Those coincide only while someone keeps the private reference in sync by hand — and when only one shipped copy drifts, both halves of the test's comparison still agree and it passes.

ADR-1207 adds core/test/test_feature_isa_invariance.c: it drives the public API twice over one fixture — once with the host ISA, once with VmafConfiguration.cpumask disabling every SIMD flag (the --cpumask switch) — and asserts bit-identical scores across ten features. It touches no internal symbol, so there is nothing to keep in sync.

ADR-1208 is what it found on its first run. All four edge_diff_map kernels computed the per-pixel |img - blur(img)| with a float subtract and promoted afterwards:

const __m512 d1 = |_mm512_sub_ps(a1, am1)|;   double ed1 = (double)d1f[k];

while the scalar reference — and each kernel's own scalar tail — promote first and subtract in double, which is exact for two floats. One call could mix both conventions depending on where a pixel fell relative to the vector width.

Nine of the ten features already passed the new gate, which is useful evidence in itself: the contract was real and one implementation was violating it.

Type

  • fix — bug fix
  • test — test-only
  • sycl / cuda / simd — backend-specific

Checklist

  • Commits follow Conventional Commits.
  • make format && make lint is green locally (pre-commit run --files clean; assertion-density, check-copyright, check-state-md-rows, check-dispatch-registry pass).
  • Unit tests pass: meson test -C build — 133/133 in a CPU-only container build.
  • If I touched any SIMD/GPU code path, I ran /cross-backend-diff and the worst ULP is ≤ 2 — the equivalent is this PR's own gate: SIMD vs scalar is now bit-identical (delta exactly 0) for all ten features.
  • If I touched a feature extractor with SIMD/GPU twins, I either updated every twin or listed the gap — all four SIMD kernels (AVX2, AVX-512, NEON, SVE2) updated identically.
  • If I added a new .c / .h, it has the appropriate license header — test_feature_isa_invariance.c carries the Copyright 2026 Lusoris header.
  • If this is a breaking change — not a breaking change.
  • If this PR adds an ADR, the row lives in docs/adr/_index_fragments/ — concat-adr-index.sh --check is in sync.

Bug-status hygiene (ADR-0165)

  • docs/state.md updated in this PR with a row — T-SSIMULACRA2-EDGE-DIFF-FLOAT-SUBTRACT-2026-09-06, closed.

Netflix golden-data gate (ADR-0024)

  • I did not modify any assertAlmostEqual(...) score in the Netflix golden Python tests.
  • If I believe a golden value must change — none changes. ssimulacra2 is fork-added; its snapshot in python/test/ssimulacra2_test.py was measured byte-identical to six decimals on all six assertions after the fix (mean 24.614428, min 13.816386, max 49.968184, harmonic_mean 22.904302, frame0 49.968184, frame47 37.415193), so nothing was regenerated.

Cross-backend numerical results

SIMD vs scalar, same fixture, via cpumask:

feature before after
ssimulacra2 1.601e-09 bit-identical
float_adm, float_vif, float_motion, float_ssim, float_ms_ssim, float_psnr, psnr_hvs, ciede, cambi bit-identical bit-identical

The AVX-512 and AVX2 paths agreed with each other throughout; the split was SIMD vs scalar.

Deep-dive deliverables (ADR-0108)

  • Research digest — docs/research/2033-isa-invariance-as-a-gate.md, on testing the property instead of a proxy for it.
  • Decision matrix — ## Alternatives considered in ADR-1207 (four options, including un-static-ing the scalars and #include-ing the .c) and ADR-1208 (including the double-vector variant and why it was not worth it).
  • AGENTS.md invariant note — added to core/src/feature/AGENTS.md.
  • Reproducer / smoke-test command — below.
  • CHANGELOG fragment — changelog.d/fixed/ssimulacra2-edge-diff-double-subtract.md.
  • Rebase note — docs/rebase-notes.md, "ADR-1207 / ADR-1208 — ISA invariance and the edge-diff subtraction".

Reproducer

meson setup build -Denable_cuda=false -Denable_sycl=false
ninja -C build
meson test -C build test_feature_isa_invariance

On the parent commit this fails with ISA invariance FAIL ssimulacra2 (ssimulacra2): host-isa=-37.499049494926254 scalar=-37.499049496527277 delta=1.601e-09; on this branch it passes with no skips. Needs a host with AVX2 or better — on a scalar-only host both runs take the same path and the test is a tautology that still passes.

Known follow-ups

  • The test localises a failure to a feature, not a kernel; the per-feature *_simd.c tests remain the tool for pinpointing which kernel. Worth revisiting whether those should compare against the shipped functions rather than private copies.
  • test_ssimulacra2_simd::test_edge still passes at 33x21 both before and after the fix, so it remains unable to catch this class on its own.

🤖 Generated with Claude Code

lusoris added a commit that referenced this pull request Sep 6, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch from d42e491 to ec5d61e Compare September 7, 2026 06:53
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch from ec5d61e to e4c2e39 Compare September 7, 2026 09:14
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch from e4c2e39 to e3fd1f0 Compare September 7, 2026 09:16
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch from e3fd1f0 to e4d7d0d Compare September 7, 2026 10:18
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch 2 times, most recently from a97949b to edd66e2 Compare September 7, 2026 16:08
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 7, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch from edd66e2 to 3fc2bb9 Compare September 7, 2026 16:38
lusoris and others added 3 commits September 15, 2026 22:39
…ate ISA invariance

The fork's contract is that a score does not depend on the host instruction
set. Ten `test_<feature>_simd.c` files assert bit-exactness and all pass, yet
`ssimulacra2` scored differently with and without SIMD — twice (ADR-1205, and
now this).

The structural reason is that those tests do not compare the two things that
ship. The shipped scalar functions are `static`, so each test defines its own
scalar reference and compares the kernel against that; when only one of the
shipped copies drifts, both halves of the test's comparison still agree and it
passes.

ADR-1207 adds `core/test/test_feature_isa_invariance.c`, which drives the
public API twice over one fixture — once with the host ISA, once with
`VmafConfiguration.cpumask` disabling every SIMD flag — and asserts
bit-identical scores across ten features. It touches no internal symbol, so
there is nothing to keep in sync.

ADR-1208 is what it found on its first run. All four `edge_diff_map` kernels
computed the per-pixel `|img - blur(img)|` with a float subtract and promoted
afterwards, while the scalar reference and each kernel's own scalar tail
promote first and subtract in double — exact for two floats. One call could
mix both conventions depending on where a pixel fell relative to the vector
width. `test_edge` passes before and after: it compares against its private
reference at 33x21, where the float subtraction happens to be exact.

Localised by dumping every scale-0 intermediate under both dispatch modes:
lin, xyb, dxyb, mu1, mu2, s11, s22 and s12 were all bit-identical and only the
edge accumulators differed, which named the function outright. Fixed by
folding the subtraction into the per-lane loop in double — that loop was
already scalar, so the vector subtract contributed nothing but rounding.

Verified: test_feature_isa_invariance passes for all ten features with no
skips (it fails on the parent commit), test_ssimulacra2_simd 13/13, libvmaf
suite 133/133, and the fork-added python/test/ssimulacra2_test.py snapshot is
byte-identical to six decimals on all six assertions, so no snapshot was
regenerated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the test/feature-isa-invariance branch from 3fc2bb9 to 2af72f3 Compare September 15, 2026 20:39
lusoris added a commit that referenced this pull request Sep 15, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 15, 2026
Seven `Recently closed` rows landed with a `PR #TBD` placeholder because
their PR number was not known when the branch was cut. The numbers are
known now, so the ADR-0334 rule that a row cites a numeric PR can be met:
#1367, #1368, #1369, #1370, #1371 (two rows) and #1374.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 15, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 15, 2026
Seven `Recently closed` rows landed with a `PR #TBD` placeholder because
their PR number was not known when the branch was cut. The numbers are
known now, so the ADR-0334 rule that a row cites a numeric PR can be met:
#1367, #1368, #1369, #1370, #1371 (two rows) and #1374.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 15, 2026
ADR-1207's ISA-invariance gate ran for the first time under clang and under
a sanitizer build on this branch, because #1367 was a draft and the
sanitizer lanes skip drafts. It found two defects, and both are on
unmodified `master`:

1. `float_vif` scores differ between the SIMD and scalar paths by 1.789e-08
   under clang. gcc contracts the scalar multiply-add into an FMA that
   happens to match the AVX kernel, so gcc-only runs look clean. Verified by
   building master with clang and this test alone: identical delta.
2. Driving `float_vif` through the AVX path under AddressSanitizer reports a
   heap-buffer-overflow — `convolution_f32_avx_s_1d_h_scanline` reads 32
   bytes starting 3 bytes past the end of a 1,966,080-byte row buffer,
   reached from the shipped `float_vif` extractor, not from a test harness.
   It aborts the process, which is why master's run never reached the
   remaining features.

Neither is a regression from this branch. I checked that directly rather
than assuming: restoring master's `vif.c`, `vif_avx2.c`, `vif_avx512.c`,
`convolution_avx*.c` and `alignment.c` onto this branch left the delta
byte-identical, and master built with clang reproduces it on its own.

`float_vif` is excluded from the gate's table with both bug ids inline. The
other nine features stay covered, so the gate keeps its value. The
assertion stays bit-identical: relaxing it to a tolerance is how the
ssimulacra2 defect in ADR-1208 survived its own SIMD test.

Verified: gcc fast suite 135/135, clang 1/1, ASan+UBSan 1/1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lusoris

lusoris commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the pre-rc.1 fixing train in #1425, which carries this work commit for commit.

Closing rather than leaving it open, because the ADR Collision Guard — a required check — fails #1425 while both PRs claim the same ADR number. The guard is right that two open PRs claim it; it just cannot tell that one of them contains the other.

Verified before closing, not assumed:

The branch is untouched, so this is reversible — reopen if #1425 is abandoned.

@lusoris lusoris closed this Sep 15, 2026
lusoris added a commit that referenced this pull request Sep 15, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 15, 2026
Seven `Recently closed` rows landed with a `PR #TBD` placeholder because
their PR number was not known when the branch was cut. The numbers are
known now, so the ADR-0334 rule that a row cites a numeric PR can be met:
#1367, #1368, #1369, #1370, #1371 (two rows) and #1374.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 15, 2026
ADR-1207's ISA-invariance gate ran for the first time under clang and under
a sanitizer build on this branch, because #1367 was a draft and the
sanitizer lanes skip drafts. It found two defects, and both are on
unmodified `master`:

1. `float_vif` scores differ between the SIMD and scalar paths by 1.789e-08
   under clang. gcc contracts the scalar multiply-add into an FMA that
   happens to match the AVX kernel, so gcc-only runs look clean. Verified by
   building master with clang and this test alone: identical delta.
2. Driving `float_vif` through the AVX path under AddressSanitizer reports a
   heap-buffer-overflow — `convolution_f32_avx_s_1d_h_scanline` reads 32
   bytes starting 3 bytes past the end of a 1,966,080-byte row buffer,
   reached from the shipped `float_vif` extractor, not from a test harness.
   It aborts the process, which is why master's run never reached the
   remaining features.

Neither is a regression from this branch. I checked that directly rather
than assuming: restoring master's `vif.c`, `vif_avx2.c`, `vif_avx512.c`,
`convolution_avx*.c` and `alignment.c` onto this branch left the delta
byte-identical, and master built with clang reproduces it on its own.

`float_vif` is excluded from the gate's table with both bug ids inline. The
other nine features stay covered, so the gate keeps its value. The
assertion stays bit-identical: relaxing it to a tolerance is how the
ssimulacra2 defect in ADR-1208 survived its own SIMD test.

Verified: gcc fast suite 135/135, clang 1/1, ASan+UBSan 1/1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lusoris
lusoris deleted the test/feature-isa-invariance branch September 18, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant