ffi_safe: pass only the base pointer through DSP fn ptrs, derive the offset from the element ptr - #1501
Open
garymm wants to merge 1 commit into
Open
Conversation
…offset from the element ptr Every DSP function pointer carried one or two trailing FFISafe arguments so that the Rust fallback could recover a `Rav1dPictureDataComponentOffset` (a 16-byte pointer-plus-offset struct) while the assembly ignored them. On x86-64 the integer argument registers are exhausted by the real arguments, so each struct became a `byval` stack copy: LLVM built it with two 8-byte stores and then copied it into the outgoing argument area with one 16-byte vector load. That load cannot be forwarded from the two pending stores and stalls for the store-to-load-forwarding penalty on every call. `perf stat` showed 42.9M store-forward blocks for rav1d against 14.6M for C dav1d on the 720p Chimera clip, most of them attributed to `recon::mc`. Pass only the base pointer (`*const FFISafe<T>`, one register or one stack slot) and let the fallback recompute the offset from the element pointer it already receives, via a new `FFISafe::with_offset_of`. No struct crosses the boundary any more. Asm-visible arguments are unchanged; only the trailing Rust-only `_`-prefixed arguments change. Chimera 720p 8-bit, `--limit 1000 --threads 1`, interleaved runs against main (d3d1cd6): - Sapphire Rapids (Xeon Platinum 8488C): 2.360s -> 2.324s (C dav1d 2.241s); `ld_blocks.store_forward` 42.9M -> 15.6M (C: 14.2M). - 4-vCPU Xeon @ 2.10GHz VM, no hardware counters: paired per-round ratio 0.987 (95% CI 0.976..0.998 over 60 rounds); callgrind instruction count -0.38%. Possible regressions to look for -------------------------------- - aarch64 is expected to be neutral. With the integer registers already used up (e.g. `mc` has nine real arguments), AAPCS64 stores the old 16-byte struct straight into the outgoing argument area with one `stp` and never reloads it, so the store-forwarding stall that x86-64 pays for the `byval` copy (four 8-byte stores followed by 16-byte `movups` reloads) does not occur there. The change saves two stores and 16 bytes of stack per DSP call on aarch64, which is likely within noise. Verified from the generated asm of a model of the call shape, not measured on arm64 hardware. The earlier `WithOffset<*const FFISafe>` shape (memorysafety#1418, memorysafety#1431) was tuned for arm64 stack usage. - The no-asm build is the path that actually executes `with_offset_of`; it is covered by the checked-release no-asm test run below. Testing ------- - `cargo build --release`; `test.sh -r target/release/dav1d -s ...`: Ok 1527 / Fail 0. - `cargo build --profile checked-release` (DisjointMut overlap checker on); `test.sh ... -t 4`: Ok 1527 / Fail 0; `test.sh ... -t 4 -f 1`: Ok 792 / Fail 0. - `cargo build --profile checked-release --no-default-features --features bitdepth_8,bitdepth_16`; `test.sh ... -t 4`: Ok 1527 / Fail 0. - `cargo fmt --check`; `cargo +stable clippy -- -D warnings` and `cargo clippy -- -D warnings` on nightly-2026-02-05: clean. - Bit-exact against C dav1d (`--muxer md5 --limit 300`) on Chimera 720p 8-bit and 1080p 10-bit at 1 and 8 threads. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FrZCpDQiJuThk7BRVsURRr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every DSP function pointer carried one or two trailing FFISafe arguments so that the Rust fallback could recover a
Rav1dPictureDataComponentOffset(a 16-byte pointer-plus-offset struct) while the assembly ignored them. On x86-64 the integer argument registers are exhausted by the real arguments, so each struct became abyvalstack copy: LLVM built it with two 8-byte stores and then copied it into the outgoing argument area with one 16-byte vector load. That load cannot be forwarded from the two pending stores and stalls for the store-to-load-forwarding penalty on every call.perf statshowed 42.9M store-forward blocks for rav1d against 14.6M for C dav1d on the 720p Chimera clip, most of them attributed torecon::mc.Pass only the base pointer (
*const FFISafe<T>, one register or one stack slot) and let the fallback recompute the offset from the element pointer it already receives, via a newFFISafe::with_offset_of. No struct crosses the boundary any more. Asm-visible arguments are unchanged; only the trailing Rust-only_-prefixed arguments change.Chimera 720p 8-bit,
--limit 1000 --threads 1, interleaved runs against main (d3d1cd6):ld_blocks.store_forward42.9M -> 15.6M (C: 14.2M).Possible regressions to look for
mchas nine real arguments), AAPCS64 stores the old 16-byte struct straight into the outgoing argument area with onestpand never reloads it, so the store-forwarding stall that x86-64 pays for thebyvalcopy (four 8-byte stores followed by 16-bytemovupsreloads) does not occur there. The change saves two stores and 16 bytes of stack per DSP call on aarch64, which is likely within noise. Verified from the generated asm of a model of the call shape, not measured on arm64 hardware. The earlierWithOffset<*const FFISafe>shape (perf:fn rav1d_cdef_brow: reduce stack usage #1418, perf: replaceFFISafe<WithOffset<..>>withWithOffset<FFISafe<..>>#1431) was tuned for arm64 stack usage.with_offset_of; it is covered by the checked-release no-asm test run below.Testing
cargo build --release;test.sh -r target/release/dav1d -s ...: Ok 1527 / Fail 0.cargo build --profile checked-release(DisjointMut overlap checker on);test.sh ... -t 4: Ok 1527 / Fail 0;test.sh ... -t 4 -f 1: Ok 792 / Fail 0.cargo build --profile checked-release --no-default-features --features bitdepth_8,bitdepth_16;test.sh ... -t 4: Ok 1527 / Fail 0.cargo fmt --check;cargo +stable clippy -- -D warningsandcargo clippy -- -D warningson nightly-2026-02-05: clean.--muxer md5 --limit 300) on Chimera 720p 8-bit and 1080p 10-bit at 1 and 8 threads.