Skip to content

PROTOTYPE (do not merge): the 32-bit index axis is half the instantiation surface - #94

Draft
balbasty wants to merge 1 commit into
mainfrom
proto/dispatch-autocast
Draft

PROTOTYPE (do not merge): the 32-bit index axis is half the instantiation surface#94
balbasty wants to merge 1 commit into
mainfrom
proto/dispatch-autocast

Conversation

@balbasty

Copy link
Copy Markdown
Collaborator

This is a prototype for discussion, not a merge candidate. One module is
converted so the numbers are real. Do not merge it as-is; decide the scope
first. It rebases on 81c60fc.

What this measures

The brief was "can autocast be simplified, and can the dispatch boilerplate be
reduced". Both turn out to be the same question, and the answer is a number
rather than a style preference.

Every templated kernel below the dispatch layer is templated on offset_t.
offset_t has exactly two values, chosen per call by canUse32BitIndexMath.
So the 32-bit index path costs exactly ×2 instantiations of everything — on
CUDA that is ×2 device code, hence ×2 ptxas memory and ×2 SASS. autocast.h
exists solely to feed that second instantiation.

Against a reg_flow that peaks at 12.98 GB of a 16 GB runner today (PR #87's
measured table), with no -arch flags and before half/bf16, that axis is the
most expensive thing in the build.

Measured — reg_flow, the binding constraint

clang++ -O3 -fPIC, BOUNDFLAGS set to the CUDA shipping policy
(-DFF_STATIC_BOUNDS=0 -DFF_STATIC_BOUND_DCT2=1 -DFF_STATIC_BOUND_DST2=1).
"instantiations" counts defined text/weak symbols in the templated impl layer —
the set ptxas would have to process on the CUDA side.

variantinstantiationsobjectpeak RSScompile CPUruntime cost
main10,13418,952 KB1,126 MB184.0 s
A. no 32-bit index5,038 (−50.3%)9,661 KB (−49.0%)624 MB (−44.6%)91.7 sunmeasured
B.BOUNDFLAGS all-Dynamic3,495 (−65.5%)8,106 KB (−57.2%)521 MB (−53.7%)78.8 sDCT2/DST2 lose their static path
C. TU split by ndim (3D arm)3,720 (−63.3%)7,862 KB (−58.5%)507 MB (−55.0%)78.1 snone
A + B1,744 (−82.8%)4,314 KB (−77.2%)346 MB (−69.3%)42.5 sas A + B

Measured — reg_field, and what the template driver itself costs

variantinstantiationsobjectpeak RSScompile CPU
main9,56619,075 KB1,043 MB196.8 s
main + no 32-bit (macro edit only)4,770 (−50.1%)9,572 KB602 MB99.2 s
this PR's driver, index axis kept9,566 (±0.0%)18,917 KB (−0.8%)1,045 MB (+0.2%)202.8 s (+3.0%)
this PR's driver + FF_INDEX32=04,770 (−50.1%)9,523 KB (−50.1%)599 MB (−42.6%)97.9 s (−50.3%)

Two things follow, and they should be decided separately:

  1. The template driver is instantiation-neutral. Identical leaf count,
    marginally smaller object, peak RSS flat, ~3% more front-end CPU. It does
    not regress memory (the failure mode worth worrying about), and it does
    not fix it either. It is a readability change, nothing more.
  2. FF_INDEX32=0 is the whole win, and it does not need the driver — the
    "macro edit only" row is main with 26 use_32bits ? … : … ternaries
    collapsed by hand, and it lands on exactly the same 4,770 instantiations.

What is in the diff

  • include/fastfields/api/dispatch.h (new) — the ndim × bound × dtype ×
    index-width pyramid stated once. dispatch_nbd<Op>(key, args…) replaces
    NDIM_SWITCH, BOUND_SWITCH, BND1/BND2/BND3, one <OP>_DT per entry
    point and one <OP>_ARGS per entry point. Arguments become an ordinary call
    argument list, so the GNU args... named-variadic-macro syntax goes with
    them (MSVC's traditional preprocessor rejects it).
    FF_INDEX32 makes the index axis one build flag instead of ~40 hard-coded
    ternaries across 21 translation units.
  • include/fastfields/core/autocast.hIndexArray<offset_t>, the RAII
    form of the copy_if_needed / free_if_needed pair, with a small stack
    buffer instead of an allocation. Fixes the leak the manual form has when
    anything between the two throws (every reg_* wrapper does
    new reduce_t[...] after the copy; every CUDA launcher throws), and takes
    cudaMallocHost/cudaFreeHost off the CUDA per-call path. The header's
    justification for pinning — "so the following H2D copy can be async" — does
    not hold for the launchers that exist: 365 of the impl/cuda upload sites
    call the synchronouscopyToDevice. The old helpers are untouched.
  • src/lib-cpu/reg_field.cpp — converted. 1369 → 1036 lines.
    _field_matvec was byte-identical to _field_matvec_acc<'='> and is gone.
  • tests/dispatch/packcheck.cpp — hand-run, deliberately not globbed by any
    make target so the recorded baseline cannot move. Checks the driver hands the
    leaf the same ndim, scalar_t, offset_t and boundary-pack length the
    macros did across all 48 (ndim, bound, index-width) configurations, and that
    the dtype/ndim/bound rejection paths still throw. Passes under clang++ and
    g++, with and without FF_INDEX32.

Gate

tools/test-baseline.sh --tree . --legs default,lib --cxx clang++
13 suites, 59,886 checks, 0 failures, plus the 2 hub suites (14 checks).
Row-for-row identical to tools/test-baseline.expected.

C++11. Clean under clang++ and g++ at -std=c++11 and -std=c++14,
-pedantic, with and without FF_INDEX32. nvcc is not available here — the
CUDA half of any migration must be gated on a real build-cuda run.

Known costs

  • clang-format. Wrapping a wrapper's body in a struct re-indents it, which
    makes the whole file "changed lines" for the lint gate — and the repo's
    existing sources are not clang-format-clean (reg_flow.cpp alone would take
    2,160 replacements). A migration should therefore keep the wrappers as free
    functions and add a six-line forwarding struct per op, leaving bodies
    untouched. This PR does not do that, on purpose, so the cost is visible.
  • Template instantiation backtraces replace macro expansions in error messages.
  • splinc / posdef / distance / solve_field have differently-shaped
    pyramids and are not worth forcing into this driver.

Generated by Claude Code

…X32 knob
RFC prototype for discussion, not for merge as-is. One module converted
(src/lib-cpu/reg_field.cpp) to show what the other five of its shape would
look like.
Primary motivation is instantiation count, not line count. Every templated
kernel below the dispatch layer is templated on offset_t, offset_t has exactly
two values, and which one is used is a runtime property of the tensor
(canUse32BitIndexMath). So the 32-bit index path costs exactly x2
instantiations of everything -- on the CUDA side, x2 device code, hence x2
ptxas memory and x2 SASS in a library that is already 166 MB and whose
reg_flow module already peaks at 13.6 GB of nvcc RSS on a 16 GB runner,
before any -arch/-gencode multiplier.
include/fastfields/api/dispatch.h (new)
The ndim x bound x dtype x index-width pyramid, stated once.
dispatch_nbd<Op>(key, args...) replaces NDIM_SWITCH / BOUND_SWITCH /
BND1..3 / one <OP>_DT per entry point / one <OP>_ARGS per entry point.
Arguments become an ordinary call argument list, so the GNU `args...`
named-variadic-macro syntax (which MSVC's traditional preprocessor rejects)
goes with them. FF_INDEX32 turns the index axis into one build flag instead
of ~40 hard-coded ternaries across 21 translation units.
include/fastfields/core/autocast.h
IndexArray<offset_t>: RAII replacement for the copy_if_needed /
free_if_needed pair, with a small stack buffer instead of an allocation.
Fixes the leak the manual form has when anything between the two throws,
and takes cudaMallocHost/cudaFreeHost off the CUDA per-call path. The old
helpers are untouched; nothing else uses IndexArray yet.
src/lib-cpu/reg_field.cpp
Converted. 1369 -> 1036 lines. _field_matvec was byte-identical to
_field_matvec_acc<'='> and is gone; the seven remaining wrappers became op
structs, and the op character is a template parameter of the struct rather
than a macro per variant.
tests/dispatch/packcheck.cpp
Hand-run, not globbed by any make target, so the recorded baseline is
untouched. Checks the driver hands the leaf the same ndim, scalar_t,
offset_t and boundary-pack length the macros did, over all 48
(ndim, bound, index-width) configurations, and that the dtype / ndim /
bound rejection paths still throw.
C++11. Clean under clang++ and g++ at -std=c++11 and -std=c++14, -pedantic,
with and without FF_INDEX32.
@balbasty

Copy link
Copy Markdown
CollaboratorAuthor
  • Use pragma once instead of guards
  • Should the autocast changes be their own PRs?
  • THe content of dispatch seems highly linked to the content of reg_field (the template order and signature is unlikely to be the same in other files), so naming it simply "dispatch" sounds a bit misleading.
    • Can we write more generic dispatcher?
    • If not, should we have dispatch utils files per API surface?

balbasty added a commit that referenced this pull request Aug 20, 2026
#151 recorded what FF_INDEX32 costs on the unsplit modules. This is the same
question asked of a slice, because the interesting case is whether splitting
makes the axis cheap enough that the knob stops mattering.
It does not. Same TU, index axis toggled:
reg_flow_matvec_3d FF_INDEX32=1 1.94 GiB 277.1 s 17.0 MB
FF_INDEX32=0 1.05 GiB 174.4 s 10.0 MB
-45.8% peak, which is the same proportion #94 measured on the unsplit file with
the host compiler (-44.6%). The split and the knob multiply rather than
overlap: 12.98 -> 1.94 GiB from one, 1.94 -> 1.05 GiB from the other.
@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Taking the five points in order. Short version: you are right on (3), and the
measurement behind it also settles (4) as no and (5) as yes. (1) is done,
and (2) is yes — this PR is three unrelated things and I have split it.

Two things changed underneath this PR and both matter for what is left of it:

I also have nvcc available locally this time, which the original run did
not. That is what turned up the core/autocast.h bug in (2) below; the
runtime claims still need a GPU, and I flag those as unverified where they
appear.


1. #pragma once instead of guards

Done. #145 made this repo-wide with core/dlpack.h the one documented
exception, and the prototype's #ifndef FF_API_DISPATCH predated it. The new
header opens with #pragma once on line 1.

All three checkers are clean on both replacement branches:

normalise-header-guards.py --check CLEAN
normalise-include-delimiters.py --check CLEAN (public includes as <fastfields/…>, #146)
rename-macros.py --check CLEAN (FF_-prefixed macros, #91)
check-cuda-launches.py --check CLEAN (#154)
codespell CLEAN

2. Should the autocast changes be their own PR?

Yes. This PR is three unrelated things — an instantiation measurement, an
autocast bug fix, and a dispatch experiment — and only the first two are
worth anything on their own. The measurement has since landed as #143. So:

The IndexArray work goes on its own, and it is not a refactor — it fixes a
leak.
Every dispatch wrapper in the tree has the shape

constoffset_t * _size = copy_if_needed<offset_t*>(size, n); // allocates
... as_weights(), newreduce_t[], the impl call ... // can throw
free_if_needed<int64_t*>(_size); // skipped

and on the CUDA side the middle section throws by design — every
FF_CUDA_LAUNCH does, and so does every copyToDevice. Reproduced under
ASan/LSan against the real core/autocast.h on the reg_field wrapper shape:
32 bytes in 2 allocations escape per throwing call on the narrow arm. It is
a per-call leak on a path user code is expected to hit (an invalid argument),
not a once-per-process one. IndexArray is RAII, so the throwing path is
covered by construction, and it keeps the elements inside the object for the
sizes that actually occur (nbatch + ndim + 1, single digits everywhere), so
the narrow arm stops allocating at all. That last part is what takes
cudaMallocHost — a page-locking syscall, 3–5 times per launch — off the CUDA
per-call path.

Two corrections to what this PR originally claimed about that header:

  • It said pinning is unjustified because "365 of the impl/cuda upload sites
    call the synchronouscopyToDevice". The direction is right, the count was
    not, and main has moved: today it is 397 synchronous copyToDevice
    against 31 stream-ordered copyToDeviceAsync
    . The async variant did not
    exist when this PR was written, and its own comment is the better argument
    anyway — it documents that a copy from pageable host memory stages through
    a driver buffer and does not return until that staging copy is done. So
    neither path requires the source to be page-locked, and the case no longer
    rests on a sync/async headcount.
  • Having nvcc locally turned up a real latent bug in the same header:
    core/autocast.h uses std::runtime_error in hostNew/hostDelete
    without including <stdexcept>.
    It compiles today only because every
    translation unit that reaches it happens to include <stdexcept> first; a
    standalone .cu that includes just that header fails outright. One line,
    folded into the same PR since it is the same file and the same work found it.

3. "dispatch" is a misleading name — is it really reg_field-shaped?

You are right, and more strongly than the comment puts it. I went and
measured the pyramid across every surface before answering (4) and (5), because
the answer to those is a number rather than a preference.

Axis order, outermost to innermost, and the leaf's template-argument order:

surfaceTUsaxes resolved, outer → innerleaf template order
reg_field / reg_flow_rls)6ndim(3) → bound(8, expanded to an ndim-long pack) → dtype(2) → index(2)<ndim, [op,] scalar_t, offset_t, bound::type...>
pushpull / pushpull_backward4ndim(3) → order(8) → bound(8) → dtype(2) → index(2)<ndim, spline::type, bound::type, scalar_t, offset_t>
resize / restrict4ndim(3) → order(8) → bound(8) → dtype(2) → index(2)as pushpull, but literal bounds
splinc2dtype(2) → index(2) → npoles(3) → bound(5)<npoles, bound::type, scalar_t, offset_t>
posdef2C(1,2,3,dyn) → dtype → index; and dtype → index<C, scalar_t, offset_t> / <scalar_t, offset_t>
distance2three different pyramids in one file<scalar_t,offset_t> / <ndim,scalar_t,offset_t> / <ndim,scalar_t,index_t,offset_t>
solve_field1dtype → index<scalar_t, offset_t>

Eight distinct leaf template-argument orders across seven surfaces. The
specific ways they refuse to line up:

  • Only the regularisers expand the boundary condition into an ndim-long
    pack
    . Everywhere else it is a single value. The pack length is load-bearing
    (bound::getutils<B> / <B,B> / <B,B,B> detect isotropy from it).
  • splinc resolves dtype and index width outermost; every other surface
    resolves them innermost. A dispatcher fixed in axis order cannot express it.
  • distance's mesh pyramid has an extra dtype axis (the face-index type,
    8 arms, signed and unsigned) and puts offset_tlast, not third.
  • The boundary axis exists in four mutually incompatible forms: the reg
    pack, pushpull's single FF_BOUND_<NAME>, resize/restrict's single
    literalbound_t::…, and splinc's 5-of-8 literal subset. The literal
    ones do not route through Dynamic, i.e. resize, restrict and splinc
    do not honour BOUNDFLAGS at all
    . I don't think that's deliberate, but it
    is load-bearing for what follows.
  • Even insidereg_field.cpp the leaf order is not uniform: nine entry
    points thread a compile-time char op as the second template argument and
    seven do not.

So api/dispatch.h was exactly as mis-named as you say. It is also a name
collision waiting to happen: core/dispatch.h already exists and holds the
genuinely surface-independent helpers (FF_VOIDPTR, FF_CANUSE32BITS,
FF_CHECK_*, off32_t, FF_INDEX32). A second dispatch.h under api/
would read as its sibling while meaning something much narrower.

Now the number that decides (4) and (5). Counting the dtype × index-width
fan — the innermost 2×2 that is the one thing every surface does have — by
where the copies actually are:

fansoff32_t mentions
the 6 regulariser TUs52104
the other 15 TUs, combined1936
whole tree71140

73% of the duplication is in one surface, because that surface copies the fan
once per entry point rather than once per module — thirteen times in
reg_field.cpp alone. Every other surface has one to three copies total.
Within the regularisers the rest is near-exact: BND1/BND2/BND3 and
BOUND_SWITCH are byte-identical in all six TUs, and NDIM_SWITCH differs
in exactly one word — the noun in its diagnostic, "field" versus "flow".

4. Can we write a more generic dispatcher?

No — and I'd argue against it even though it is possible. Four reasons, in
descending order of how much I believe them:

  1. The deduplication yield is close to zero. A dispatcher generic over all
    seven surfaces needs a per-surface trait naming the axis list, the mapping
    from axis values to leaf template-argument positions, and the accept/reject
    sets (splinc's 5-of-8 bounds, mesh's 2-of-3 ranks). That trait is per surface
    and about as long as the pyramid it replaces. Meanwhile the measured
    redundancy is 52 of 71 fans inside a single surface, which a per-surface
    header captures completely. The generic machinery would exist to capture the
    19 remaining fans spread across six surfaces that have one to three each.
  2. C++11 makes the general case expensive in exactly the wrong currency. No
    if constexpr, no fold expressions, no enable_if_t, no generic lambdas,
    and no index_sequence (that is C++14, and the CPU/hub layers are C++11). A
    dispatcher generic in the axis list is recursive over that list, and each
    node is a class-template instantiation per axis prefix. Note that this PR's
    own driver deliberately avoided that — it hand-wrote three
    _expand_bound specialisations rather than recursing, with the comment that
    the recursive form "instantiates ndim + 1 class templates … per (op, ndim,
    bound) triple, which is pure front-end cost for no benefit" — and it still
    only handled one fixed axis list. Generalising re-introduces precisely what
    it removed.
  3. It cannot be sold on memory, and I want to be explicit about that rather
    than imply otherwise.
    I re-measured on current main: a driver of this
    shape is exactly instantiation-neutral. reg_field.cpp, clang++ -O3 -fPIC with the CUDA shipping bound policy, counting defined text/weak
    symbols — 10,140 before, 10,140 after, object 19,533,368 → 19,533,360
    bytes. Leaf codegen is what ptxas processes, and it does not move. The only
    honest case for a generic dispatcher is readability and MSVC portability.
  4. It would quietly change instantiation policy. Any unified boundary axis
    has to pick one treatment, and resize/restrict/splinc currently use
    literal bounds that bypass BOUNDFLAGS while the regularisers and pushpull
    route through FF_BOUND_<NAME>. Unifying them changes what gets
    instantiated — in one direction that is a memory regression, in the other a
    behaviour change. Either way it is not a refactor, and given that any
    increase in instantiation count is a non-starter, it is the wrong thing to
    bundle into one.

On MSVC specifically, since the PR raised it: the GNU named-variadic
args... syntax is a genuine blocker and it is orthogonal to this question —
it is fixed by spelling the macros with ISO ... / __VA_ARGS__, which the
new header does. But I want to be plain that this is a down payment, not a fix:
MSVC's traditional preprocessor also mis-forwards __VA_ARGS__ into a nested
macro
(it arrives as a single argument), so a nested pyramid needs
/Zc:preprocessor regardless of how it is written. There is no MSVC in this
environment and I have not tested any of that
— it is from the documented
behaviour, not measured. If MSVC becomes a real target, that is its own
investigation and the template-driver form does side-step it, which is the one
argument for the driver I still find persuasive.

5. Dispatch utils files per API surface?

Yes — and the tree has already converged on this twice, independently of this
PR.

  1. api/cpu/pushpull_dispatch.h and api/cuda/pushpull_dispatch.h are
    already merged and are exactly this: one header per surface, shared by that
    surface's two TUs, #pragma once, FF_-prefixed macros. Worth noting that
    the two backend copies are macro-identical — they differ only in comment
    text and one #include line (impl/cpu/pushpull.h vs
    impl/cuda/pushpull.h). Whether that pair should collapse further is a
    separate question I have not touched.
  2. perf(cuda): split reg_flow into per-(family, ndim) translation units #147 independently arrived at the same answer while splitting reg_flow:
    its src/lib-cuda/reg_flow_slice.inl defines FF_FLOW_BND1/2/3,
    FF_FLOW_MV_DT, FF_FLOW_DG_DT, … — the same pyramid, renamed per surface.

So the pattern is established; what is missing is the regulariser one, which is
where 73% of the duplication lives. Naming and placement, following pushpull:

  • include/fastfields/api/<backend>/reg_dispatch.h<surface>_dispatch.h,
    matching pushpull_dispatch.h, so the name says which surface it belongs to
    and cannot be mistaken for core/dispatch.h.
  • api/, not core/. Per the definition PROPOSAL (do not merge): what core/ means, and a naming scheme #149 settled on and you agreed to,
    core/ holds what more than one layer needs and that is not the computation
    of a named fastfields operation
    . A header that names one operation family's
    leaf templates is tied to a named operation, so it belongs beside the entry
    points whose leaves it names.
  • One header for reg_fieldandreg_flow rather than one each, because
    their pyramids were byte-identical apart from that one diagnostic noun, which
    becomes a parameter.

What I am opening instead of this PR

Three independently reviewable pieces. Each says what it is worth alone.

  • core/autocast.h: IndexArray — fixes the leak-on-throw (32 bytes / 2
    allocations per throwing call, reproduced under LSan), stops the narrow arm
    allocating for the ranks that actually occur, takes cudaMallocHost off the
    CUDA per-call path, and adds the missing <stdexcept>. Converts the eight
    non-regulariser surfaces on both backends (228 call sites, 15 TUs). Worth
    landing even if the dispatch question is answered "leave it alone".
  • api/cpu/reg_dispatch.h — the answer to (3) and (5): 52 fans → 2,
    BOUND_SWITCH and NDIM_SWITCH → one each, −473 lines across the two CPU
    regulariser TUs. Measured instantiation-neutral (10,140 → 10,140). Worth
    landing as readability only; it makes no memory claim.
  • The CUDA mirror of that header is deliberately not in it, because
    src/lib-cuda/reg_flow.cpp is being rewritten by perf(cuda): split reg_flow into per-(family, ndim) translation units #147. That half should
    follow perf(cuda): split reg_flow into per-(family, ndim) translation units #147 rather than collide with it, and perf(cuda): split reg_flow into per-(family, ndim) translation units #147's FF_FLOW_* macros are
    the natural thing to point at the shared header once both have landed.

This PR should be closed rather than reworked in place — its headline
finding landed as #143, its api/dispatch.h is superseded by the per-surface
header, and its autocast half is now its own change. I have left the branch
alone; say the word and I will close it, or retitle it as the record of the
measurement if that is more useful.

What I dropped from it, and why

  • The template-driver form (a struct per op, dispatch_nbd<Op>(key, …)).
    It is instantiation-neutral, so it buys no memory, and its own "known costs"
    section is right that wrapping wrapper bodies in structs re-indents them and
    makes the whole file changed-lines for the format lint. The macro header gets
    the same deduplication with a diff that is almost entirely deletions.
  • tests/dispatch/packcheck.cpp — it existed to prove the driver handed
    the leaf the same ndim/scalar_t/offset_t/pack length the macros did.
    The replacement does not need it: the pack macros are the same tokens in a
    different file, so there is nothing new that could disagree.

Gate

tools/test-baseline.sh --legs default,lib --check tools/test-baseline.expected
→ I will post the exact rows on each PR rather than assert them here. CUDA is
compile-verified locally with nvcc 12.0 as well as in CI, but there is still no
GPU here, so anything about runtime behaviour on the CUDA side — including
whether dropping cudaMallocHost changes upload throughput — is unmeasured and
I am not claiming it either way.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

The two replacements are open:

Neither carries this PR's api/dispatch.h or its packcheck.cpp, and neither
touches the index axis — #143 settled that.

The CUDA mirror of #158's header is the obvious third piece, but it wants
src/lib-cuda/reg_flow.cpp, which #147 is rewriting, so it should follow #147
rather than race it. #147's reg_flow_slice.inl has grown its own FF_FLOW_*
copy of the same pyramid; pointing that at the shared header is the natural
merge point once both land.

Happy to close this one whenever you like.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Two corrections to my long reply above, both found by checking my own numbers
rather than by anyone catching them. Neither changes an answer.

1. The op / no-op split in reg_field is 8 and 5, not "nine and seven".
Counted properly, of the thirteen dispatch sites per regulariser file:

sitesentry points
thread a compile-time char op8addmatvec_, submatvec_, and diag / kernel in all three =/+/- variants
no op at all5plain matvec, relax, matvec_rls, diag_rls, relax_rls

The point it was supporting is unaffected — the leaf order is still not uniform
withinreg_field.cpp, which is what made a single fan impossible and is why
#158's header writes it twice. Fixed in the header comment and in #158's
description.

2. The leak in (2) is less reachable on the CPU than I implied. I said it is
"a path user code is expected to hit — an invalid argument". It is not: every
FF_CHECK_* throw fires in the exported entry point, before dispatch reaches
the wrapper that allocates. So a bad argument does not leak. The accurate
version:

  • CPU — needs std::bad_alloc between the pair. Effectively unreachable.
  • CUDA — the impl call throws for ordinary runtime conditions: every
    FF_CUDA_LAUNCH since fix(cuda): route all 39 kernel launches through one checked helper #154, and every copyToDevice. Device OOM and
    cudaErrorLaunchOutOfResources leak the narrowed arrays, page-locked ones
    under FF_AUTOCAST_PINNED_HOST.

Which also explains why CI is green on main today: the ASan/LSan leg only
covers tests/lib-cpu/, where the throwing path needs bad_alloc that no test
induces, and the CUDA path where the throw is routine has no runtime coverage
at all. The fix is still worth having — it is the device-OOM path that leaks —
but "expected to hit" was too strong. Written out on #159.


Status: #158 is green on every check CI runs for it, including
lint (clang-format, changed lines), which I gather has been red elsewhere.
#159's test-cpu legs, test-hub and compile-probe-cuda are green;
both build-cuda legs are still running, and that PR should not move until
they report since it touches core/.

One more thing came out of the measurement and is filed separately as #160:
resize, restrict and splinc do not respond to BOUNDFLAGS / SPLINEFLAGS
at all — identical instantiation counts under all-static and all-dynamic,
against −97.2% for pushpull — because they name bound_t::Zero directly
instead of the FF_BOUND_* selectors. Those three are 3.72 GB of CUDA build
memory that the policy currently cannot touch. It is a question rather than a
bug report; nothing in #158 or #159 depends on the answer.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

One point of precision on my answer to (5), because "yes, dispatch utils per API
surface" can be read as "so make seven of them", and I do not think that is what
the evidence supports.

A per-surface header earns its place only where a surface has more than one
copy of the pyramid to remove.
By that test, exactly two surfaces qualify, and
one already has its header:

surfacedispatch TUsfansheader warranted?
regularisers652yes#158
pushpull (+backward)42yes — already exists
resize / restrict42no — 1 fan each, and they are separate ops
distance23 each, all different shapesno — see below
posdef22no
splinc21no
solve_field11no

For the bottom five, a header would move one switch out of one file and add an
include. That is ceremony, not deduplication, and it would make the dispatch
harder to read rather than easier — the pyramid is currently right next to the
entry point it serves, which for a single-fan module is where it belongs.

distance is the interesting one and the clearest argument against
mechanically applying the rule: it has three pyramids, but they are three
genuinely different shapes (<scalar_t,offset_t> for euclidean/l1,
<ndim,scalar_t,offset_t> for spline, <ndim,scalar_t,index_t,offset_t> for
mesh) serving three different operations that happen to share a file. Hoisting
them into a distance_dispatch.h would produce a header containing three
unrelated things — the same mistake as api/dispatch.h, one level down.

So the rule I would actually propose is narrower than "one per surface":

Hoist a dispatch pyramid into api/<backend>/<surface>_dispatch.h when two
or more translation units
must agree on it. Otherwise leave it beside its
entry points.

That is precisely what pushpull_dispatch.h's own docstring already says its
reason is — the forward and backward TUs "must nevertheless agree exactly on
which (order, bound) combinations are statically instantiated". The regularisers
are the other case that meets it (six TUs, and #147 is currently growing a
seventh copy). Nothing else does.

It also happens to be the rule that predicts where the bugs would be: a macro
duplicated across TUs that must agree is the configuration that produced the
three preserved divergences and the two-different-macros-one-name collision
documented in core/dispatch.h. A macro used once in one file cannot drift.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Closing the loop on the one claim in this PR that was an extrapolation rather
than a measurement — and it turns out CI has been measuring it since #143
landed, on real nvcc, post-#155.

This PR argued the index axis costs "×2 device code, hence ×2 ptxas memory and
×2 SASS", from clang++ host-compiler counts. It could not check that: no nvcc
here at the time, and the CUDA library was inert anyway until #155.

#143's two-leg matrix now builds the same tree twice with only FF_INDEX32
differing, which is a controlled experiment for exactly this. Wall time of the
build-cuda legs on main:

runindex64index32ratio
3242033464218 min40 min2.2×
3240173037920 min38 min1.9×
#159 (in flight)22 minrunning

Roughly 2×, reproduced across independent runs. So the shape of the original
claim holds up when actually measured on nvcc — the axis really is close to a
straight doubling of the CUDA compile, not a marginal cost.

Three caveats, because this is easy to over-read:

  • Wall time is not ptxas memory. They are correlated here but not the same
    thing, and peak RSS is what the 16 GB runner constraint is actually about.
    The build-cuda job prints per-module peak RSS on every run ("Peak nvcc
    memory per module"), so the memory version of this table is available; I could
    not pull the job logs from this environment (the redirect to blob storage is
    blocked), so I have not tabulated it.
  • -j2 overlap muddies wall time, per the note above MODULES in
    src/lib-cuda/Makefile — two heavy modules overlapping changes the total in
    ways that are not a clean function of instantiation count.
  • None of this argues for removing the axis. You have decided to keep 32-bit
    dispatch and I am not reopening that. What it does mean is that FF_INDEX32
    is a real lever with a measured price, which is the useful form for that
    finding to survive in — and it is already documented as one in
    core/dispatch.h and both Makefiles.

Which is the last thing I wanted to check before recommending this PR be closed:
its headline is now both superseded (the knob exists, #143) and
independently confirmed (the two legs measure it every run). There is
nothing left in it that is not either landed, replaced by #158/#159, or
deliberately dropped.


Generated by Claude Code

Sign up for freeto 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.

2 participants

@balbasty@claude