Skip to content

refactor: de-duplicate the dispatch helpers into shared headers - #90

Merged
balbasty merged 2 commits into
mainfrom
refactor/dedup-shared-macros
Aug 20, 2026
Merged

refactor: de-duplicate the dispatch helpers into shared headers#90
balbasty merged 2 commits into
mainfrom
refactor/dedup-shared-macros

Conversation

@balbasty

Copy link
Copy Markdown
Collaborator

De-duplicates the copy-pasted helper macros and functions in the two
dtype-dispatch layers and the hub into shared headers, giving them
collision-safe FF_ names as they move.

This is PR 1 of 2. The follow-up
(refactor/prefix-public-macros) prefixes the remaining unprefixed public
macros, FF itself included. De-dup first, because it is the smaller and more
substantive change, and separating them makes a regression trivially
bisectable.

What was duplicated

macro / functioncopieswhere
VOIDPTR, CANUSE32BITS, CHECK_NO_LANES19 eachsrc/lib-cpu (8–10) + src/lib-cuda (9) + both api/*/pushpull_dispatch.h
CHECK_SAME, CHECK_SAME_DTYPE17 eachsame
CVOIDPTR11same
CHECK_SAME_BATCH10same
CHECK_SAME_SHAPE9same
IS_CPU, IS_CUDA9 eachsrc/lib (hub)
as_weights(), _reg_stream()3, 4reg_field* / reg_flow*

Note the counts are 19/17, not the 17/15 in the original audit: VOIDPTR and
friends were already on the installed public surface via
include/fastfields/api/{cpu,cuda}/pushpull_dispatch.h. They were never purely
TU-private, which strengthens rather than weakens the case for the rename.

Three of them had genuinely diverged

Not formatting — behaviour:

macrothe divergencenow
CVOIDPTRposdef.cppalone guards a null data; an absent optional operand arrives as a descriptor with data == nullptr, and offsetting a null pointer is UBFF_CVOIDPTR_OR_NULL
CHECK_SAME_BATCHdistance.cpp / posdef.cppalone also reject ndim < D — they derive D from an operand's own ndim and would otherwise read past the end of shape[]FF_CHECK_SAME_BATCH_ND
CHECK_SAME_SHAPEtwo different macros sharing one name — a 2-arg whole-shape check in distance.cpp, a 3-arg leading-D check in the regularisers and solve_fieldFF_CHECK_SAME_SHAPE / FF_CHECK_SAME_SHAPE_N

The last one is the argument for doing this at all. With 17 private copies, one
name silently meant two things, and nothing in the tree could have told you.
Each divergence keeps its own name so the difference is visible at the call
site rather than hiding in one file's prologue.

(CHECK_NO_LANES and CHECK_SAME_DTYPE also had two spellings each — those
really are line-wrapping only, and the proof below confirms it.)

Where things went, and why

Placement is by audience, and the audience question that bites is which
compiler sees the code
.

new/changedcontentswhy there
include/fastfields/core/dispatch.h(new)FF_VOIDPTR, FF_CVOIDPTR, FF_CVOIDPTR_OR_NULL, FF_CANUSE32BITS, the FF_CHECK_* family, as_weights()src/lib-cuda is compiled by nvcc while src/lib-cpu and src/lib use the host compiler. Anything all three share must be backend-agnostic, and core/ is the only directory that is so by contract. api/checks.h is host-only and was the wrong home.
include/fastfields/api/cuda/stream.h(new)_reg_stream()Names cudaStream_t, so CUDA-only by construction. Also keeps CI's path filter honest — in core/ it would have triggered the CPU legs for nothing.
include/fastfields/api/checks.h(extended)is_cpu() / is_cuda() inline functionsUsed only by the host-compiled hub. A function in ff:: is collision-safe with no prefix at all, so these two macros are deleted rather than renamed.

core/dispatch.h deliberately does not wrap the checks in
do { … } while (0), and says so: the 17 copies did not, and wrapping them
would silently change which statements a brace-less
if (cond) FF_CHECK_…(…); guards. Tightening that is a behaviour change and
belongs in its own commit, measured against the CPU suite.

The naming rule applied

Every macro that survives preprocessing of a header under include/ — every
#define not #undef'd before the end of the header that defined it — must
be spelled FF_*.

Macros private to a single .cpp are exempt: they never leave the translation
unit. But the moment one is hoisted into a header it stops being private
which is exactly why de-duplicating and prefixing are one change and not two.
Where an inline function will do, prefer it and delete the macro outright
(IS_CPU/IS_CUDA here; uchar_t in PR 2).

Evidence: proved, not asserted

tools/macro-equivalence.py preprocesses every (file, macro) pair on both
sides of the change and compares token streams:

$ git worktree add --detach /tmp/base HEAD~1
$ python3 tools/macro-equivalence.py /tmp/base .
OK(parens) src/lib-cpu/posdef.cpp CVOIDPTR -> FF_CVOIDPTR_OR_NULL
OK(parens) src/lib-cuda/posdef.cpp CVOIDPTR -> FF_CVOIDPTR_OR_NULL
121 macro expansions compared across 23 files: 119 exact, 2 balanced-paren-only, 0 MISMATCHED

The two non-exact ones are FF_CVOIDPTR_OR_NULL, which composes FF_CVOIDPTR
and so carries one extra balanced parenthesis pair; the tool only accepts
that after checking the token streams are identical once parentheses are
removed. Everything else matches exactly.

A whole-translation-unit token diff was tried first and is the wrong
instrument — core/dispatch.h pulls in <vector> and makes as_weights()
visible everywhere, which swamps the signal with additive, behaviour-free
noise. Comparing the expansions in isolation asks the question that matters.

Reviewing this

Do not read 1100 renamed call sites. Re-derive them:

git checkout HEAD~1 -- include src
python3 tools/dedup-dispatch-helpers.py
git diff # must be empty

Both scripts are committed and idempotent for exactly this reason.

Validation

  • make test CXX=clang++ — the CPU suite, 59,886 checks across 13 suites,
    0 failures
    , unchanged. (Results are in the review comment below once the
    clean-worktree run finishes.)
  • codespell clean.
  • CUDA is compile+link only, as always — no GPU here, so build-cuda in CI is
    the bar. core/dispatch.h correctly triggers the full matrix.

Note on the clang-format check

It will report against this PR, and its report will be empty. The job is
broken, independently of this change
: git-clang-format-18 --diff exits 1
when the diff is non-empty, and the step runs it under set -euo pipefail in a
command substitution, so the script dies at the assignment before reaching
the case that would print the diff. That is why #85 (docs-only) shows a bare
exit 1 with no output. It is continue-on-error: true, so it does not block.
Reproduced locally; details in the task report. Not fixed here — the workflow
file is already being edited by #87.

For the record, the tree is not clang-format-clean on main either
(src/lib-cpu/splinc.cpp alone differs by 154 lines), which is why that job is
advisory in the first place.


Generated by Claude Code

Every src/lib-cpu/<module>.cpp and src/lib-cuda/<module>.cpp opened with the
same private prologue: pointer marshalling macros, a 32-bit-index predicate,
and a family of argument checks. There were 19 copies of VOIDPTR, 19 of
CANUSE32BITS, 19 of CHECK_NO_LANES, 17 of CHECK_SAME and 17 of
CHECK_SAME_DTYPE, plus 9 copies each of the hub's IS_CPU / IS_CUDA, three of
as_weights() and four of _reg_stream().
They had already drifted. Three of them are not a formatting difference:
CVOIDPTR posdef.cpp alone guards against a null `data`, because an
absent optional operand arrives as a descriptor with
data == nullptr and offsetting a null pointer is UB.
CHECK_SAME_BATCH distance.cpp and posdef.cpp alone also reject ndim < D,
because they derive D from an operand's own ndim and would
otherwise read past the end of shape[].
CHECK_SAME_SHAPE was TWO DIFFERENT MACROS SHARING ONE NAME -- a 2-argument
whole-shape check in distance.cpp, a 3-argument leading-D
check in the regularisers and solve_field.
That last one is the argument for doing this at all: with 17 private copies,
one name silently meant two things, and nothing could have told you.
WHAT MOVED WHERE
include/fastfields/core/dispatch.h (new)
FF_VOIDPTR, FF_CVOIDPTR, FF_CVOIDPTR_OR_NULL, FF_CANUSE32BITS, the
FF_CHECK_* family, and as_weights().
In core/ because src/lib-cuda is compiled by nvcc while src/lib-cpu and
src/lib use the host compiler: anything all three share has to be
backend-agnostic, and core/ is the only directory that is so by
contract. api/checks.h is host-only and was the wrong home.
include/fastfields/api/cuda/stream.h (new)
_reg_stream(). Names cudaStream_t, so it is CUDA-only by construction
and belongs under api/cuda/ -- which also keeps CI's path filter honest.
include/fastfields/api/checks.h (extended)
is_cpu() / is_cuda() inline functions, replacing IS_CPU / IS_CUDA. Used
only by the host-compiled hub. A function in ff:: is collision-safe with
no prefix at all, so these two are deleted rather than renamed.
All three preserved divergences keep their own name -- FF_CVOIDPTR_OR_NULL,
FF_CHECK_SAME_BATCH_ND, FF_CHECK_SAME_SHAPE_N -- so the difference is visible
at the call site instead of hiding in one file's prologue.
NAMING
A macro that lives in a .cpp cannot collide with anything; a macro in an
installed header is a name taken from every downstream translation unit. So
the moment one of these was hoisted it had to be prefixed, which is why
de-duplicating and prefixing are one change and not two. (VOIDPTR and friends
were in fact already on the public surface via api/{cpu,cuda}/
pushpull_dispatch.h, which the audit of "17 copies in .cpp files" had missed;
the true counts are 19 and 17.)
EVIDENCE
Not asserted, proved. tools/macro-equivalence.py preprocesses every
(file, macro) pair on both sides of the change and compares token streams:
121 macro expansions compared across 23 files:
119 exact, 2 balanced-paren-only, 0 MISMATCHED
The two are FF_CVOIDPTR_OR_NULL, which composes FF_CVOIDPTR and so carries one
extra balanced parenthesis pair; the tool checks the token streams are equal
once parentheses are removed. A whole-translation-unit token diff was tried
first and is the wrong instrument -- core/dispatch.h pulls in <vector> and
makes as_weights() visible everywhere, which swamps the signal with additive,
behaviour-free noise.
The rewrite itself is tools/dedup-dispatch-helpers.py, committed so that the
~1100 renamed call sites can be re-derived and diffed rather than read.
@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Gate: unmoved

test-cpu (clang-dynamic) on this PR, per-suite, straight from the job log:

suitechecksfailures
distance23520
distance_mesh46220
distance_spline7040
posdef40120
pushpull3080
pushpull_backward63810
reg_field192500
reg_flow163470
reg_op1860
resize6300
restrict650
solve_field4520
splinc45770
13 suites59 8860

Every row matches tools/test-baseline.expected exactly. Not just "the suite
passed" — the check counts are identical, which is the property that would
catch an assertion silently disappearing.

test-hub green too (5 + 9 = 14 assertions), and compile-probe-cuda green —
so nvcc accepts the changed headers.

Layered evidence

  1. Macro expansions — 121 (file, macro) pairs preprocessed on both sides,
    token streams compared: 119 exact, 2 balanced-paren-only, 0 mismatched.
    Re-runnable: python3 tools/macro-equivalence.py <old-tree> .
  2. The rewrite — re-derivable: git checkout HEAD~1 -- include src && python3 tools/dedup-dispatch-helpers.py && git diff is empty.
  3. The gate — the table above.
  4. CUDA — compile + link only, as always. No GPU here; nothing about
    runtime CUDA behaviour is claimed.

The clang-format check is broken — independently of this PR

It reports failure here with no diff printed, same as on #85 (docs-only).
Root cause, reproduced locally with the pinned clang-format 18:

$ git-clang-format --diff --extensions h,hpp,inl,cpp,cu,cuh HEAD~1
EXIT=1 outlen=406 # exits 1 when the diff is NON-EMPTY
$ bash -c 'set -euo pipefail; o=$(git-clang-format --diff ... HEAD~1); echo "CASE REACHED"'
ci-construct exit=1 # "CASE REACHED" never prints

The step runs out=$(git-clang-format-18 --diff …) under set -euo pipefail.
git-clang-format --diff exits 1 whenever it has something to say, so the
script dies at the assignment before reaching the case that would echo
$out. The job therefore reports a formatting failure while showing you
nothing — the one thing that would let you act on it.

One-line fix, for whoever owns it: out=$(… || true), or bracket the
assignment with set +e / set -e. Not done here.github/workflows/ci.yml
is already being edited by #87, and this is not that PR's business either way.

It is continue-on-error: true, so it blocks nothing. Worth knowing that the
tree is not clang-format-clean on main regardless (src/lib-cpu/splinc.cpp
alone differs by 154 lines), which is why the job is advisory — a mechanical
rename in a hand-column-aligned tree was always going to light it up.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Local clean-worktree run finished, confirming the CI numbers on a second machine and toolchain invocation:

$ make -j2 test CXX=clang++ # clean tree, dedicated worktree
MAKE_EXIT=0
13 suites | 59886 checks | 0 failures

Per-suite, identical to tools/test-baseline.expected row for row:
2352 / 4622 / 704 / 4012 / 308 / 6381 / 19250 / 16347 / 186 / 630 / 65 / 452 / 4577.

Also verified: core/dispatch.h, api/cuda/stream.h and api/checks.h each
compile standalone (clang++ -fsyntax-only on a TU that includes only that
header), so the new headers are genuinely self-contained rather than relying on
their first includer.


Generated by Claude Code

@balbastyClaude

Copy link
Copy Markdown
CollaboratorAuthor

Validation status at hand-off

gate#90#91
test-cpu (clang-static)
test-cpu (clang-dynamic)
test-cpu (clang-cuda-default)
test-cpu (gcc-static)
test-cpu (sanitize asan+ubsan)
test-hub
compile-probe-cuda (nvcc)
lint (codespell)
lint (clang-format)❌ — job is broken, see above❌ — same
build-cuda (compile + link)still runningstill running
local make test (clean worktree)✅ 13 / 59 886 / 0✅ 13 / 59 886 / 0

All four boundary/spline compile policies agree with the reference oracle, ASan

  • UBSan are clean, and nvcc already accepts the changed impl/kernels and
    impl/cuda headers via compile-probe-cuda.

build-cuda is the one gate not yet observed green — it is the ~35-minute
full nvcc compile+link of src/lib-cuda, and it was still running at hand-off.
It is the leg that matters most for these headers (half the de-duplicated
macros live in nvcc-compiled sources), so please confirm it on the PR rather
than taking the rest of this table as sufficient. No claim is made about CUDA
runtime behaviour — there is no GPU in CI and compile+link is the accepted
bar.


Generated by Claude Code

Keeps both additions to CLAUDE.md: main's corrected CUDA memory note (the
measured 12.98 GB reg_flow peak, replacing the stale ~3.8 GB figure) and
this branch's FF_-prefix rule for macros in installed headers.
@balbasty
balbasty merged commit ed2738b into mainAug 20, 2026
10 of 11 checks passed
@balbasty
balbasty deleted the refactor/dedup-shared-macros branch August 20, 2026 00:24
balbasty pushed a commit that referenced this pull request Aug 20, 2026
Brings in #87, #90, #91 and #95. One conflict, in
include/fastfields/impl/kernels/parallel.h: #91 renamed
FF_NAMESPACE_BEGIN(FF) to FF_NAMESPACE_BEGIN(FF_NS) on the line this branch
inserts the FF_GRAIN_SIZE block above. Resolved by keeping both -- the new
block, then main's FF_NS spelling.
Everything else auto-merged. #91's renames do not touch anything this branch
depends on: has_atomic_add / anyAtomicAdd keep their names, FF_NS still
expands to ff, and the CUDEV -> FF_CUDEV rename is confined to the CUDA half
of atomic.h. FF_GRAIN_SIZE, the one macro this branch adds to an installed
header, already satisfies #91's FF_-prefix rule --
`tools/rename-macros.py --check` reports "0 file(s) would change" and
"include/ is clean".
Re-verified on the merge result:
* tools/test-baseline.sh --legs default,lib -> byte-identical to
tools/test-baseline.expected. 13 suites, 59,886 checks, 0 failures.
* -DFF_GRAIN_SIZE=1 -> 59,886 / 13 / 0.
* -DFF_GRAIN_SIZE=1 + TSan, FF_NUM_THREADS=4, halt_on_error=1
-> 59,886 / 13 / 0, zero reports.
* clone syscalls: 0 across all 13 binaries at the shipping grain size,
2 per binary at FF_GRAIN_SIZE=1. The threshold is unchanged by the merge
(0 clones at n=32768, 2 at n=32769, on main and on this branch alike).
* The thread-pool defects still reproduce on main at f63c7d8: the data race
is deterministic under TSan (threadpool.h:148 write / :164 read) and the
lost-wakeup deadlock is stochastic (8/320 trials over FF_NUM_THREADS
8/16/32/64). Both are gone on this branch: 0/320 hangs, 0 TSan reports.
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