Skip to content
This repository was archived by the owner on Aug 20, 2026. It is now read-only.

fix: widen the public stream parameter to intptr_t - #61

Merged
balbasty merged 2 commits into
mainfrom
claude/4-cuda-stream-intptr_t
Aug 2, 2026
Merged

fix: widen the public stream parameter to intptr_t#61
balbasty merged 2 commits into
mainfrom
claude/4-cuda-stream-intptr_t

Conversation

@balbasty

@balbastybalbasty commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Part of #4 (tracked in fastfields-lib: fastfields/fastfields-lib#4). Repo 3 of 5.

Why this repo is in the series at all

The CPU backend never dereferences stream — nothing here is broken at runtime. But these declarations are half of the hub's device-dispatch ABI. ff::dt_euclidean and friends in fastfields-lib inspect the DLTensor's device and forward the same argument to either FF_CPU:: or FF_CUDA:::

if (is_cuda) returnFF_CUDA::dt_euclidean(inp_out, voxel_spacing, stream);
elsereturnFF_CPU::dt_euclidean(inp_out, voxel_spacing, stream);

Widening only the CUDA side would leave the hub unable to call both backends with one signature. So this is a signature-only change kept in lockstep with the rest of the series.

What changed

116 parameter declarations across the 8 modules (distance, posdef, pushpull, pushpull_backward, reg_field, reg_flow, resize, restrict, splinc), plus the <cstdint> includes they need. Existing column alignment is preserved (intptr_t is 5 characters wider than int, so the following padding is shortened to match).

The diff contains no non-signature lines — verified mechanically:

$ git diff -U0 -- '*.h' '*.cpp' | grep '^[-+]' | grep -v '^[-+][-+]' \
| grep -viE 'stream|#include <cstdint>'
(empty)

How this was verified

Really run, twice, on the same machine:make test CXX=clang++, once on pristine main and once with this change.

baseline (main)this branch
make exit00
suites PASSED12 / 1212 / 12
total checks4267842678
failures00

Per-suite counts are byte-identical (diff of the running / checks: / PASSED lines is empty), which is what you would expect from a change that alters no CPU code path:

test_distance 2352 · test_distance_mesh 4622 · test_distance_spline 704 · test_posdef 4012
test_pushpull 308 · test_pushpull_backward 6381 · test_reg_field 7426 · test_reg_flow 11415
test_reg_op 186 · test_resize 630 · test_restrict 65 · test_splinc 4577

impl was pinned to the SHA this repo's main already records (b6eae48) for both runs, so the comparison isolates this change.

PR series

  1. fix: widen the CUDA stream parameter to intptr_t and forward it in the distance launchers fastfields-cuda-impl#25
  2. fix: widen the CUDA stream parameter to intptr_t and forward it for the distance ops fastfields-cuda-lib#35
  3. fastfields-cpu-lib — this PR
  4. fix: widen the hub's public stream parameter to intptr_t + bump cpu/cuda pins fastfields-lib#50
  5. fix: widen the CUDA stream parameter to intptr_t at the nanobind boundary fastfields-dlpack#22

Workstream: claude-jitfields-to-fastfields

🤖 Generated with Claude Code

A `cudaStream_t` is a pointer and therefore 64-bit, but every public
`stream` parameter was declared `int`. The CPU backend never dereferences
the value, so nothing here is broken at runtime -- but these declarations
are half of the hub's device-dispatch ABI: `ff::dt_euclidean` and friends in
fastfields-lib forward the same argument to either `FF_CPU::` or `FF_CUDA::`,
so the two backends must agree on its type. Widening only the CUDA side
would leave the hub unable to call both with one signature.
This is a signature-only change: 116 parameter declarations across the 8
modules, plus the `<cstdint>` includes they need. No CPU code path reads the
value, and the diff contains no non-signature lines.
Verified with `make test CXX=clang++`: 12/12 suites PASSED, 42678 checks,
0 failures -- byte-identical per-suite check counts to a pristine `main`
run of the same suite on the same machine.
Refs fastfields/fastfields-lib#4
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
@balbasty
balbasty merged commit 660f980 into mainAug 2, 2026
4 of 5 checks passed
@balbasty
balbasty deleted the claude/4-cuda-stream-intptr_t branch August 2, 2026 08:47
@balbastyClaude

Copy link
Copy Markdown
ContributorAuthor

Review summary (triage, jitfields-to-fastfields): merged as squash.

Signature-only widening (intintptr_t, 116 params across 8 modules), mechanically verified as touching no non-signature lines. Held for ~2h behind the RLS broadcast regression (cpu-lib#62) since it shares this repo — re-verified with a fresh branch update against the now-fixed main rather than trusting the PR's original (stale, pre-fix) local baseline. All 3 bound-policy matrix legs (static/dynamic/cuda-default) genuinely green post-update. clang-format shows a failure but is non-blocking as of cpu-lib#63 — expected and harmless.

Workstream: claude-jitfields-to-fastfields


Generated by Claude Code

balbasty added a commit to fastfields/fastfields-dlpack that referenced this pull request Aug 2, 2026
…dary (#22)
* fix: widen the CUDA stream parameter to intptr_t at the nanobind boundary
Closes#4.
A `cudaStream_t` is a pointer and therefore 64-bit, but every `stream`
parameter accepted at this boundary was declared plain `int` in the
nanobind lambda signatures. A real stream handle from a caller (e.g. a
torch/cupy stream pointer forwarded as an integer) whose upper 32 bits are
non-zero is silently truncated before it ever reaches the C++ library --
the last and most externally visible link in the truncation chain fixed
by this issue across all five repos. Widens all 45 `stream` parameters in
`src/ext.cpp` to `intptr_t`.
Bumps the `_fastfields_lib` submodule pin to fastfields-lib's
16f3eb7 (fastfields/fastfields-lib#50), which carries the matching widening
through the hub and both device-dispatch layers
(fastfields/fastfields-cpu-lib#61, fastfields/fastfields-cuda-lib#35) down
to the CUDA launchers themselves (fastfields/fastfields-cuda-impl#25).
Verified end to end on this machine (CPU path; no GPU available):
- `pip install --no-build-isolation .` succeeds: builds `_core.so` via
nanobind against the bumped `_fastfields_lib`, links `-lfastfields`,
and installs the wheel.
- `import fastfields.dlpack._core` succeeds and exposes the expected
entry points (dt_euclidean, dt_l1, dt_mesh, field_matvec, ...).
- Regression check for the actual bug: calling `dt_euclidean(x, 1.0,
1 << 40)` -- a stream value with bits set above the 32-bit range --
completes without truncation or OverflowError, which a plain `int`
parameter could not guarantee.
- `pytest tests/`: 36 passed, 0 failed.
Refs fastfields/fastfields-lib#4
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016AjQcY78NgbagPSbPJRr6Z
* fix: bump _fastfields_lib pin past the RLS broadcast-stride fix
fastfields-cpu-lib#62 (field_matvec_rls's RLS/shared-weight broadcast
path reading the wrong stride, corrupting output for wc=1, C>1) has
now been fixed and threaded all the way up through cpu-impl#48,
cuda-impl#27, cpu-lib#64, cuda-lib#38, and fastfields-lib#55. This
repo's _fastfields_lib pin (16f3eb7) predated fastfields-lib#55, so
the fix was not reachable through this PR's pin -- the extension
built here would still have linked against the broken
field_matvec_rls.
Bumps _fastfields_lib -> fd9e6c5 (fastfields-lib main HEAD, which now
carries cpu -> 4d46a02 and cuda -> e96440c, both past the fix).
Verified for real:
CXX=clang++ pip install --no-build-isolation .
cd /tmp && python -m pytest <repo>/tests -q
Install succeeds end to end (builds libfastfields.so/-cpu.so via the
bumped _fastfields_lib's Makefile, links the nanobind extension,
packages and installs the wheel). pytest: 36 passed, 0 failed. Also
re-ran the stream-widening regression check this PR introduced
(stream = 2**40 via dt_euclidean) against the freshly built
extension: still passes.
part of fastfields-cpu-lib#62
Co-authored-by: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@balbasty@claude