PROPOSAL (do not merge): what core/ means, and a naming scheme

[balbasty]

This is a design proposal, not a migration. It is a draft and should
stay one until the owner decides. The second commit is a prototype kept
only so the cost figures are measurements; drop it and the design still
stands.

(Replaces #148, which carried the same content on a branch with a
malformed commit trailer. Same two commits, rebased onto main after #146
and #143 landed.)

Answers the two questions asked together, plus the thread-count question
that follows from them. Full document:
docs/proposals/core-and-naming.md.

Recommended definition

core/ holds every header that more than one layer depends on and that is
not itself the computation of a named fastfields operation — device-specialised
or not. impl/kernels/ keeps only the per-element maths of a named operation.

It decides every borderline file: it admits atomic.h (device-specialised, but
not an operation) and the parallel/threadpool group, and it excludes splinc.h
(which is the operation exported as spline_coeff).

Two premises in the brief that do not hold

• core/ was said to be barred from opening the device namespace.
  core/autocast.h already opens FF_NAMESPACE_BEGIN(FF_DEVICE), and
  core/cuda_switch.h exists to branch on __CUDACC__ and pulls
  cuda_fp16.h under nvcc. The real constraint is narrower: a core/
  header must compile under both compilers and mean the right thing in each.
  So no fourth directory, and no core/cpu + core/cuda split.
• core/dispatch.h lines 28 and 48 say canUse32BitIndexMath comes from
  core/autocast.h. It is defined at impl/kernels/utils.h:668. That is
  why 17 files in src/ include the kernels layer directly — the dependency
  inversion this proposal repairs is load-bearing today and mis-documented.

Also: the header-guard PR (#145) landed the opposite of what the brief
described — guards everywhere, no #pragma once — and existing guards keep
their names, so a file move needs no guard renames.

The file list

Ten files, 4,262 lines. Closure is clean and checked mechanically: core/
never acquires a dependency on impl/kernels/.

utils.h · bounds.h · spline.h · batch.h · atomic.h · meta.h ·
parallel.h · parallel_impl.h · threadpool.h · threadpool.inl

core/ goes 5 → 15 files; impl/kernels/ goes 57 → 47 and then holds only
per-operation implementations.

Not moved, with reasons in the doc:

• vector/ (11 files, 2,389 lines) — not infrastructure, superseded.
  Its only includer is a scratch target that is compiled but never run, and
  distance/mesh_utils.h contains an independent parallel hierarchy of the
  same abstraction. Decide its fate separately.
• tetrahedron.h — this is not a placement question. Both copies are
  referenced by nothing, and impl/cpu/tetrahedron.h does not compile
  (uses parallel_for/GRAIN_SIZE with no includes, and sits in ff::tetra
  rather than ff::cpu::tetra). Nothing builds it, so nothing catches it.
• splinc.h — the owner's instinct is right, and the one-character gap
  from spline.h is the trap worth naming out loud.

Naming

The vox idea is right for a stronger reason than "marking voxelwise-ness":
the kernels layer and the impl layer share a namespace — all seven module
namespaces are opened by both. It does not collide today only because every
kernel entry point is hidden inside a class template (Kernels, RegFlow,
Multiscale, Child) rather than being a free function. Two pieces of
evidence that this workaround is load-bearing: impl/cpu/pushpull.h lines
37–82 contain a lambda named pull shadowing the function pull it sits
inside, and teeny's commit b7dbd08 fixed exactly this collision with
exactly this vox. Recommended spelling: ff::⟨device⟩::⟨module⟩::vox —
26 sites across 25 files. elem is the more accurate name by the codebase's
own prose; the doc gives the trade rather than pretending it is settled.

atomic.h is the case that tests the scheme, and it is a real bug, confirmed
by brace-depth scan: the whole CUDA branch sits at global scope, including
an overload of CUDA's own built-in atomicAdd (line 263) and ATen's
AtomicFPOp / gpuAtomicAdd names verbatim — in an installed header, with
fastfields-torch as a planned downstream consumer. FF_GPU_ATOMIC_INTEGER
is also defined, never used, and never #undef'd.

On internal helpers: the _ sigil marks two unrelated things (116 distinct
identifiers, 2,943 mentions — overwhelmingly narrowed locals like
_stride_out, and only incidentally functions). Keep it for locals; for
functions adopt the convention #147 is already landing
(ff::cuda::flow_slice::, a named nested namespace, no sigil) rather than
inventing a competing one. Anonymous namespaces will not serve here, though
all 21 dispatch TUs already use them — one nested in ff::cuda cannot hold a
flow_matvec alongside the exported ff::cuda::flow_matvec without making
unqualified calls ambiguous.

Thread-count API

ff::set_num_threads / get_num_threadsalready exist, fully
implemented, at threadpool.inl lines 58–69; parallel_impl.h already wraps
them in a backend-abstracted pair covering the OpenMP and single-threaded
builds. The task is export, not design. Three things a public setter would
expose: the obvious public name is already taken by the internal helper;
set_parallel_threads(-1) reaches ThreadPool(size_t(-1)) and spawns threads
until the process dies; and the setter mutates two function-local statics with
no lock, in a pool where #97 has just fixed races reachable only at higher
thread counts. The /2 default is a deliberate x86-only SMT correction — keep
it, document it, and note that CI therefore runs the pool at 2 threads.

CUDA: do not mirror it. But the real finding is not the API — all 39
launch sites use CUDA_NUM_THREADS = 1024 (the architectural maximum) and
cudaGetLastError/cudaPeekAtLastError appear zero times anywhere, so a
cudaErrorLaunchOutOfResources is discarded silently and the output tensor
keeps whatever it held. Adding launch error checking is the highest-value item
in this document and is independent of everything else in it.

Gate

Recorded in a comment below.
tools/move-core-headers.py --check → clean, no dependency leak, idempotent.

Sequencing

Must land after#145 (#146 has now landed). The script does not hard-code
an include delimiter — it measures the tree's dominant one — so it emitted
quotes before #146 and angle brackets after, unedited. That was demonstrated,
not assumed: #146 and #143 landed while this was being written, and the
rebase was done by re-running the script, exactly as its docstring prescribes.

Verified independent: #147 touches src/lib-cuda/ only (16 files, none
under include/) — zero overlap, and its flow_slice:: seam is an input to
the naming section rather than a conflict. #143 intersected only on the 17
src/*.cpp whose utils.h include line changes — same files, different
lines, and the merge was clean. CI is neutral: ci.yml:102 already puts
impl/kernels/ and core/ in the same "trigger EVERYTHING" clause.

Everything in the naming section should wait and land as separate PRs;
bundling a rename into a relocation makes both unreviewable.

---

Generated by Claude Code

[balbasty]

Gate

Measured with tools/test-baseline.sh --legs default,lib --check tools/test-baseline.expected, clang++.

Result — row-for-row identical to tools/test-baseline.expected:

suite	config	checks	failures
distance	default	2352	0
distance_mesh	default	4622	0
distance_spline	default	704	0
posdef	default	4012	0
pushpull	default	308	0
pushpull_backward	default	6381	0
reg_field	default	19250	0
reg_flow	default	16347	0
reg_op	default	186	0
resize	default	630	0
restrict	default	65	0
solve_field	default	452	0
splinc	default	4577	0
lib_device_check	lib	5	0
lib_splinc_bound	lib	9	0

13 cpu-lib suites, 59,886 checks, 0 failures, plus the hub's 2 suites / 14 checks.

--check exits non-zero reporting BASELINE NOT COMPARABLE on leg coverage
(2 of the 6 recorded legs were run), not on any differing row. The four legs not
run — static, dynamic, sanitize, cuda-default — vary only the
BOUNDFLAGS/SPLINEFLAGS compile policy, which this change does not touch.

What was verified on which tree — stated precisely

That run was on the pre-rebase tree (base 85fdac7, quoted includes). The
branch as it now stands is the same move re-applied on de288a9 after #146 and
#143 landed; the two trees differ only in include-delimiter spelling
("fastfields/…" → <fastfields/…>), which is #146's already-merged change and
resolves to the same headers through -I include either way. A confirming run
on the current tree was still building when this was written — CI on this PR is
the authoritative check, and it exercises all the legs rather than two.

tools/move-core-headers.py --check on the current tree:

clean; 10 header(s) in include/fastfields/core,
include delimiter <...>, no dependency leak

Idempotent, and the closure check is what guarantees core/ has not acquired a
dependency on impl/kernels/.

---

Generated by Claude Code

[balbasty]

lint (clang-format, changed lines) fails, and that is the most useful thing this PR found

Not a defect in the prototype — a property of file moves under this gate that
would have bitten whoever implemented the migration. Writing it up here because
it changes the scheduling.

Mechanism

The job runs

git-clang-format-18 --diff --extensions h,hpp,inl,cpp,cu,cuh $(git merge-base origin/main HEAD)

and git-clang-format does not honour rename detection. It sees
include/fastfields/core/bounds.h as a path that did not exist at the merge
base, treats the whole file as changed, and requires all 800 lines to conform —
for a file that predates the gate and has never been clang-formatted.

Measured

	reformat lines demanded
the 10 moved files	4,232
impl/kernels/resize.h (a genuinely changed include line)	4
everything else	0

4,232 of 4,236 are the rename artifact. The genuine formatting debt this
change creates is four lines.

It is not fixable by making the move cleaner

I tried. f982e6c tightens the script so it leaves alone any include whose
existing spelling still resolves from the new location, and spells the rest
absolutely — matching what core/ already does for its own siblings
(core/dispatch.h → <fastfields/core/autocast.h>), which also stops the move
fighting #146. Result: all ten moved files are now byte-identical pure
renames, verified by checksum, and the diff drops from 150/150 to 140/140
across 82 files, with every include edit falling on the 71 files that did not
move.

The linter is unmoved. A zero-content-change rename is flagged exactly as hard —
the 4,232 figure above is measured after that tightening.

(The tightening is worth keeping regardless: pure renames are what make the
review of a relocation possible at all.)

Mitigation, verified

clang-format the ten files in place, in impl/kernels/, as its own PR,
before the move. Formatting a file drops it off the linter's list entirely —
confirmed by formatting one of them and watching it disappear from the output.
That PR is trivially green, because there the files are not renamed, so only the
lines it changes get checked and it changes them to be clean.

The cost is honest and worth stating: it reformats ~4,200 lines of bounds.h,
spline.h, utils.h and friends and churns blame across them. That is
pre-existing debt the gate has not billed yet — every one of these files is one
rename away from being billed for it, with or without this proposal.

Alternatives, both CI-side rather than source-side, both leaving the debt in
place:

• teach the lint to skip files whose diff status is R100;
• exempt the one PR.

I lean towards paying it, because a formatting-only PR is reviewable and a
combined one is not — 4,200 lines of reflow would bury the twelve include lines
that carry the actual meaning. But it is the owner's call, and it is the single
item here most likely to change how this gets scheduled. Written up as §4.1 of
the proposal.

---

Generated by Claude Code

[balbasty]

CI status on 11113c8

Job	Result
test-cpu (clang-dynamic)	✅
test-cpu (clang-index64)	✅
test-cpu (clang-cuda-default)	✅
test-cpu (sanitize asan+ubsan)	✅
test-cpu (tsan, grain=1)	✅
test-hub	✅
lint (codespell)	✅
detect changed paths	✅
test-cpu (clang-static)	running
test-cpu (gcc-static)	running
compile-probe-cuda	running (✅ on f982e6c, identical code)
build-cuda (index32) / (index64)	running
lint (clang-format, changed lines)	❌ — see the comment above; this is the finding, not a defect

Two things worth pulling out:

tsan (grain=1) is green. That is the leg that rebuilds the whole 59,886-check
suite with FF_GRAIN_SIZE=1 so it actually runs multi-threaded under
ThreadSanitizer. It is the one that matters most for this change, because
parallel.h, parallel_impl.h, threadpool.h and threadpool.inl are four of
the ten files being relocated.

compile-probe-cuda passed on f982e6c, whose C++ is byte-identical to this
SHA (the two commits since are documentation only). That is the check that
matters for the §1.1 argument: nvcc digests the relocated core/ headers. The
build-cuda legs will confirm it end-to-end, including the hub link that gates
backend completeness.

The only red is clang-format, and it is red for a reason this PR documents
rather than one it introduces. codespell — which also runs tree-wide — is
green.

---

Generated by Claude Code

[balbasty]

Update:compile-probe-cuda has now completed ✅ on 11113c8 itself, not
just on the identical-code predecessor. That is the check the §1.1 argument
actually rests on — nvcc digests the relocated core/ headers — so it is worth
noting it is no longer inherited evidence.

Nine of fourteen green: clang-dynamic, clang-index64, clang-cuda-default,
sanitize, tsan (grain=1), test-hub, compile-probe-cuda, codespell,
detect changed paths.

Still running: clang-static, gcc-static, and both build-cuda legs (the
latter are the long ones — reg_flow alone is the 12.98 GiB compile).

Unchanged: clang-format is the only red, for the reason documented above.

I am deliberately not pushing anything further to this branch — an earlier
documentation-only push cancelled the in-flight CUDA jobs and they had to start
over. Anything that needs changing here should wait until this run finishes.

---

Generated by Claude Code

[balbasty]

Agreed