Uh oh!
There was an error while loading. Please reload this page.
This repository was archived by the owner on Aug 20, 2026. It is now read-only.
pushpull: implement spline::type::Dynamic + fix two Dynamic-order/-bound bugs - #44
Merged
Merged
Conversation
… policy Mirrors the bound::type::Dynamic pattern from #42 one axis further out: pushpull templates on interpolation order as well as boundary condition, so instantiating all eight orders statically multiplies an already-large matrix by eight -- affordable on the CPU but not under nvcc's ptxas. - spline::SplineVec: runtime per-axis order vector, trivially copyable, the exact analogue of bound::BoundVec. - spline::dyn<S>: empty zero-cost forwarder to utils<S> for a real S; a separate explicit specialisation for S == Dynamic that holds the runtime order and switches on it directly (not via the existing weight_fn/bounds_fn function-pointer helpers -- those don't inline on GPU, same reasoning as bound::dyn<Dynamic>). - FF_STATIC_SPLINES / FF_STATIC_SPLINE_<NAME> policy macros and the FF_SPLINE_<NAME> selector macros dispatch layers must use instead of spline::type::<NAME> directly, so a Dynamic build routes through the shared instantiation. - Removed the old utils<type::Dynamic> specialisation: it silently returned all-zero weights and an empty node range for anything that called it, rather than failing to compile. dyn<Dynamic> replaces it as the only legitimate way to hit the runtime-order path. Part of fastfields-cuda-lib#30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
…bjects PushPullUtils::index/gindex/hindex selected between the static utils<S>/ utils<B> path and the runtime Dynamic path via function pointers (spline::weight_fn(s), bound::sign_fn(b), ...) chosen with a ternary on S == Dynamic / B == Dynamic. Indirect calls don't inline on GPU -- the same reasoning that made bound::dyn<Dynamic> switch directly instead of using bound::index_fn/sign_fn as function pointers (see bounds.h). Replace the per-call function-pointer selection with `bound::dyn<B>` / `spline::dyn<S>` member objects constructed once per index/gindex/hindex call: for a static B/S both are empty, zero-cost forwarders (the ternary and the function-pointer indirection disappear entirely at compile time); for Dynamic they hold the runtime value and switch on it directly. No behavioural change -- this only replaces how the existing Dynamic path is reached. Verified via the CPU oracle suite in fastfields-cpu-lib (test_pushpull, 308/308 under the default all-static policy). Part of fastfields-cuda-lib#30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
Both bugs live in the generic ("Any" spline order) Kernels specialisation
that every FourthOrder-SeventhOrder call (and, as of this change, every
spline::type::Dynamic axis) actually goes through -- invisible until now
because nothing previously exercised an axis where the runtime `s`/`b`
argument was read at all: the default BOUNDFLAGS/SPLINEFLAGS policy kept
every spline order statically instantiated, and a statically-instantiated
`dyn<S>`/`utils<S>` ignores its runtime argument entirely. Both were caught
by the CPU oracle suite (test_pushpull) under a Dynamic policy, which is
exactly why the project runs it under both policies rather than trusting
"compiles" as "correct" -- see fastfields-cuda-lib#30.
1. pushpull/1d.h, Kernels<Config<one, Spline<I>, Bound<B>, ABS>>::push:
the runtime bound/order (`b`, `s`) were computed but never forwarded to
`utils::index(...)` -- the call used the (wrong, for a Dynamic axis)
compile-time default arguments instead. The local `s` was also shadowed
by the following loop's own `s` (the stride variable), so even a
corrected call site would have needed the loop variable renamed too
(done here, to `st`, matching every other method in the file).
2. pushpull/3d.h, Kernels<Config<three, Spline<IX,IY,IZ>, ...>> (all nine
methods: pull/push/count/grad/hess/*_backward): the X-axis
`utils_x::index/gindex/hindex(...)` call passed `sz` (the Z-axis order)
instead of `sx` (the X-axis order) -- a copy-paste slip, most likely
from writing the Z-axis line first and not updating the X-axis one.
Wrong whenever X and Z end up with different *runtime* spline orders
under Dynamic, silently computing X's weights with Z's order instead.
Both fixed by 1:1 comparison against the working Y-axis code / the other
eight methods in the same files. Reproduced pre-fix as 32/308 test_pushpull
failures (all under SPLINEFLAGS+BOUNDFLAGS routing FourthOrder-SeventhOrder
and six of eight bounds through Dynamic); 308/308 pass after the fix under
that policy, the fully-static default, and a fully-Dynamic policy.
Part of fastfields-cuda-lib#30.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
This was referenced Jul 31, 2026
balbasty
commented
Jul 31, 2026
| // is stateless and its constructor discards the argument). Trivially copyable, | ||
| // so it can be passed by value all the way into a `__global__` kernel. | ||
| // | ||
| // `max_ndim` is 3 because every operator in the library is 1D, 2D or 3D. |
CollaboratorAuthor
There was a problem hiding this comment.
We might want to implement nd (n>3) push-pull. So can we not have such a strong limitation?
| { for (int d = 0; d < max_ndim; ++d) s[d] = static_cast<int8_t>(v); } | ||
| // Anisotropic: one order per axis (padded with `Linear`). | ||
| inline CUHOSTDEV SplineVec(const type * v, int ndim) |
CollaboratorAuthor
There was a problem hiding this comment.
Padding with linear must be well documented.
Addresses review feedback on #44: `SplineVec` hard-coded `max_ndim = 3` ("every operator in the library is 1D, 2D or 3D"), which reads as a real architectural ceiling rather than what it actually is -- today's kernels (kernels/pushpull/{1d,2d,3d}.h; `nd.h` exists but is currently unreachable dead code) just don't go past ndim=3 yet, and n-D (n>3) pushpull is wanted later. - `SplineVec` -> `template <int MaxNDim=3> struct SplineVecN {...};` + `using SplineVec = SplineVecN<3>;`. A future n>3 kernel instantiates `SplineVecN<N>` directly; every existing `spline::SplineVec` reference (cpu-impl, cuda-impl, cpu-lib, cuda-lib) is unaffected -- the alias has the exact same size/layout/behaviour as before. - Applied the identical change to `bound::BoundVec` -> `BoundVecN<MaxNDim>` + `using BoundVec = BoundVecN<3>;`. Not explicitly requested on this PR, but `BoundVec` has the exact same hard-coded `max_ndim = 3` (predates this PR, from #42) and is used by every pushpull/regulariser launcher that takes a runtime boundary condition -- generalising only the spline axis while leaving the bound axis capped at 3 would not actually unblock n>3 pushpull, since both vectors gate the same kernels. Same alias-for-back-compat treatment; zero behaviour change for existing code. - Documented the `Linear`-padding behaviour on `SplineVecN`'s anisotropic constructor per review feedback: padding axes (d >= ndim) get `type::Linear` purely so the trivially-copyable struct never holds an uninitialised/Dynamic byte; no kernel ever reads a padding axis (every dispatch loop runs exactly `ndim` times), so the pad value is inert -- `Linear` specifically because it is the cheapest real order to evaluate if a padding axis were ever (incorrectly) read, matching `BoundVecN`'s analogous choice of `type::Zero` for the same purpose. `spline.h`/`bounds.h` are backend-agnostic, header-only, and compiled under both FF_DEVICE=cpu and FF_DEVICE=cuda from the same source (see CLAUDE.md) -- confirmed no CUDA-specific override or duplicate of either struct exists anywhere in cpu-impl/cuda-impl, so this single change covers both backends. Verified: `BoundVecN<7>`/`SplineVecN<7>` instantiate cleanly alongside the existing `BoundVec`/`SplineVec` usage (syntax-check); full CPU oracle suite green (test_pushpull 308/308, test_reg_field 4610/0, test_reg_flow 5859/0, test_reg_op 186/0) after rebuilding against these headers -- zero regression from the templating. Part of fastfields-cuda-lib#30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
balbasty
commented
Jul 31, 2026
CollaboratorAuthor
Addressed both review comments (pushed to
(GitHub's GraphQL review-thread API is rate-limited for this account right now, so I couldn't reply inline / mark the threads resolved directly — leaving this as a regular comment instead.) Generated by Claude Code |
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Workstream: claude-jitfields-to-fastfields
Part of fastfields/fastfields-cuda-lib#30 (wire
pushpullinto cuda-lib's CI). This is the kernels-layer piece: aspline::type::Dynamicruntime fallback, mirroring thebound::type::Dynamicpattern from #42/#43, plus two real correctness bugs it surfaced.spline::type::Dynamicpushpulltemplates on interpolation order in addition to boundary condition, so instantiating all eight orders statically multiplies an already-large(ndim x bound x dtype x offset_t)matrix by eight again — the actual driver of the ~40 min / OOM-risk pushpull compile that has kept it out offastfields-cuda-lib'sMODULES.spline::SplineVec— runtime per-axis order vector, trivially copyable,__host__ __device__, the exact analogue ofbound::BoundVec.spline::dyn<S>— for a real staticS, an empty zero-cost forwarder tospline::utils<S>; forS == Dynamic, an explicit specialisation holding the runtime order and switching on it directly (not via the existingweight_fn/bounds_fnfunction-pointer helpers — those don't inline on GPU, same reasoning asbound::dyn<Dynamic>).FF_STATIC_SPLINES/FF_STATIC_SPLINE_<NAME>build-time policy macros andFF_SPLINE_<NAME>selector macros, mirroringFF_STATIC_BOUNDS/FF_BOUND_<NAME>.utils<type::Dynamic>specialisation: it silently returned all-zero weights and an empty node range for anything that called it, instead of failing to compile.dyn<Dynamic>is now the only legitimate way to hit the runtime-order path.pushpull/utils.h'sPushPullUtils::index/gindex/hindexnow buildbound::dyn<B>/spline::dyn<S>objects once per call instead of the previous per-field function-pointer ternaries — same behaviour, one indirection point instead of several.Assembly-diff verified (see fastfields-cuda-lib#30 for the full writeup): a fully-static spline+bound instantiation (Cubic+DCT2, and separately FourthOrder+Zero via the generic path) produces byte-identical
-O3assembly before and after this change (diff/md5summatch exactly). TheDynamicpath was confirmed to generate genuinely different (branchier, ~3x larger) code, not an accidental no-op.Two pre-existing bugs found and fixed
Both live in the generic ("Any" spline order)
Kernelsspecialisation that every FourthOrder–SeventhOrder call — and, as of this PR, everyspline::type::Dynamicaxis — actually goes through. Invisible until now because nothing previously exercised an axis where the runtimes/bargument was actually read (the default build policy kept every order static, and a staticdyn<S>/utils<S>ignores its runtime argument entirely).pushpull/1d.h, the genericKernels::push: the runtime bound/order were computed but never forwarded toutils::index(...)— it used the (wrong, for a Dynamic axis) compile-time default arguments instead. The localswas also shadowed by the following loop's owns(renamed tost, matching every other method in the file).pushpull/3d.h, all nine methods (pull/push/count/grad/hess/*_backward): the X-axisutils_x::index/gindex/hindex(...)call passedsz(the Z-axis order) instead ofsx— a copy-paste slip. Wrong whenever X and Z end up with different runtime spline orders under Dynamic.Reproduced pre-fix as 32/308
test_pushpullfailures onfastfields-cpu-lib(alladjoint_1d_orderchecks) under aBOUNDFLAGS/SPLINEFLAGSpolicy that actually routes some axes through Dynamic. 308/308 pass after the fix, under three policies: the fully-static default, the policyfastfields-cuda-libnow ships by default, and a fully-Dynamic policy.Test plan
test_pushpull(fastfields-cpu-lib): 308/308 under the default all-static policy (zero regression from thedyn<>refactor).test_pushpull: 308/308 under-DFF_STATIC_SPLINES=0 -DFF_STATIC_SPLINE_{NEAREST,LINEAR,QUADRATIC,CUBIC}=1 -DFF_STATIC_BOUNDS=0 -DFF_STATIC_BOUND_{DCT2,DST2}=1(the policy fastfields-cuda-lib now ships) — this run is what caught the two bugs; 308/308 after the fix.test_pushpull: 308/308 under a fully-Dynamic policy (-DFF_STATIC_SPLINES=0 -DFF_STATIC_BOUNDS=0).-std=c++11) of a standalonePushPullUtils::indexinstantiation: byte-identical for static Cubic+DCT2 and static FourthOrder+Zero, before vs. after this change.nvccinstantiation smoke test (see fastfields-cuda-impl's companion PR) of every pushpull op, 1D/2D/3D, under both static and Dynamic parameterisations.🤖 Generated with Claude Code
Generated by Claude Code