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.

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 is needed.
• core/dispatch.h:28,48 says 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

impl/kernels/ 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:37-82
has 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: ff::<device>::<module>::vox, 28 sites. 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 is 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.

Thread-count API

ff::set_num_threads / get_num_threadsalready exist, fully
implemented, in threadpool.inl: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, 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

Prototype commit, clang++, legs default,lib — row-for-row identical to
tools/test-baseline.expected: 13 cpu-lib suites, 59,886 checks, 0
failures, plus the hub's 2 suites / 14 checks. --check reports NOT
COMPARABLE on leg coverage (2 of 6 recorded legs), not on any differing
row; the other four legs vary only the bound/spline compile policy, which
this change does not touch.

tools/move-core-headers.py --check → clean, no dependency leak, idempotent.

Sequencing

Must land after#145 and #146. The script does not hard-code an include
delimiter — it measures the tree's dominant one — so it emits "fastfields/…"
before that sweep and <fastfields/…> after, and a rebase is resolved by
re-running it, never by hand.

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 intersects only on the
~16 src/*.cpp whose utils.h include line changes — different lines in the
same files. 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]

Superseded by #149 — same content, same two commits, rebased onto main after #146 and #143 landed, on a branch with a corrected commit trailer. Closing this one so there is a single place to review.

---

Generated by Claude Code