feat(sp14-c.5a-fixup): missing scratch buffers + collector ptr scaffolding (dead code)

Phase C.5a-fixup — completes C.5a's additive infrastructure so C.5b can be
a true atomic contract flip with zero scaffolding work mixed in:

- Allocate dh_aux1_pre_scratch [B_max, AUX_TRUNK_H1=256] +
  dh_aux2_pre_scratch [B_max, AUX_TRUNK_H2=128] (missed in C.5a — both
  required by aux_trunk_backward.launch per gpu_aux_trunk.rs:266-267).
- Collector struct gains 6× u64 aux_trunk_{w1,b1,w2,b2,w3,b3}_ptr fields,
  exp_aux_trunk_forward_ops: AuxTrunkForwardOps field (constructed in
  ctor on collector's stream), and set_trainer_aux_trunk_param_ptrs
  setter — mirrors the existing set_trainer_params_ptr zero-copy pattern.
- Trainer gains aux_trunk_param_ptrs() -> (u64×6) accessor returning
  raw_ptr() for all 6 aux trunk parameter tensors.
- training_loop wires the new setter at both existing
  set_trainer_params_ptr call sites (initial fused-ctx init + fold-boundary
  re-init).

NO contract change: wire sites still call save_h_s2. The setter is called
and the 6 aux trunk param ptrs are populated, but no collector-side launch
reads them yet — C.5b atomically inserts aux_trunk_forward.launch(...)
post-forward_online_f32 and switches the aux head input pointer.

Graph-capture audit: aux_heads_backward IS INSIDE the captured `forward`
child graph (call chain: submit_forward_ops_main → launch_cublas_backward
→ launch_cublas_backward_to → aux_heads_backward; capture begins at
fused_training.rs:2964 / capture_child_graph). The existing function body
is fully device-side (zero host writes) — capture-safe by construction.
C.5b's new aux_trunk Adam launch is also fully device-side and will sit
inside the same captured region. The host writes for aux_trunk_t_pinned
must use the existing GPU-side increment_step_counters kernel chain
(submit_counters_ops, line 22799) — NOT host-side aux_trunk_adam_step
+= 1 inside capture. ISV-driven LR/clip writes happen pre-capture
(cold-path); the captured graph reads via aux_trunk_lr_dev_ptr /
aux_trunk_grad_clip_dev_ptr. This avoids the &self → &mut self ripple on
aux_heads_backward (gap 4 in C.5b implementer's blocker report). Full
wiring strategy + alternative (pre-capture host-write) documented in
docs/dqn-wire-up-audit.md C.5a-fixup section.

Verification:
- cargo check -p ml --tests --all-targets: clean (no new warnings).
- cargo test -p ml --test aux_trunk_oracle_tests --test sp14_oracle_tests
  --release -- --ignored --nocapture: 12/12 pass (8 aux_trunk + 4 sp14;
  bit-identical to C.5a baseline — pure scaffolding, no regression).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
jgrusewski
2026-05-08 02:37:48 +02:00
parent c90de98594
commit 1edd71a2c1
4 changed files with 267 additions and 0 deletions

View File

@@ -7410,3 +7410,97 @@ C.5b is the genuine atomic contract migration:
5. Insert `aux_trunk_backward.launch(...)` + `launch_aux_trunk_adam_update(...)` calls.
6. Initialize `aux_trunk_lr_pinned` + `aux_trunk_grad_clip_pinned` from ISV per-step (host writes before each launch).
7. Increment `aux_trunk_adam_step` per Adam call.
## SP14 Layer C Phase C.5a-fixup — missing scratch buffers + collector ptr scaffolding (2026-05-08)
> Authorized 2026-05-08 after C.5b implementer flagged five gaps in C.5a:
> (1) two scratch buffers required by `aux_trunk_backward.launch` were never allocated;
> (2) collector lacked aux trunk param ptrs needed for the zero-copy contract;
> (3) graph-capture status of the C.5b new launch sites was unverified;
> (4) `aux_heads_backward` `&self``&mut self` ripple from host-side step counter (deferred to C.5b);
> (5) C.5b LOC ≈600-900 vs the 300 originally estimated (mitigated by removing scaffolding from C.5b's scope).
> C.5a-fixup addresses gaps 1, 2, 3 — additive-only DEAD CODE, no contract change. Gaps 4 + 5 are C.5b's concerns.
### Gap 1 — missing dh_pre scratch buffers (`gpu_dqn_trainer.rs`)
`aux_trunk_backward.launch` ([`gpu_aux_trunk.rs:266-267`](../crates/ml/src/cuda_pipeline/gpu_aux_trunk.rs)) requires two caller-allocated scratch buffers for the per-layer pre-activation gradient tile:
- `dh_aux1_pre_scratch` `[B_max, AUX_TRUNK_H1=256]` — Layer-1 dh_pre output of `aux_trunk_bwd_dh_pre`, consumed by `aux_trunk_bwd_dW_reduce` for `dW1 + db1`.
- `dh_aux2_pre_scratch` `[B_max, AUX_TRUNK_H2=128]` — Layer-2 dh_pre output, consumed for `dW2 + db2`.
C.5a missed these. Allocated alongside the existing C.5a saved-fwd / accumulator / grad buffers via `alloc_f32(&stream, b * h_layer, "...")`. Both fields tagged `#[allow(dead_code)]` until C.5b atomically wires the backward chain.
### Gap 2 — collector aux trunk ptr scaffolding (`gpu_experience_collector.rs`)
C.5b inserts `aux_trunk_forward.launch(...)` in the collector's per-rollout-step body so the collector-native EGF chain feeds from the aux representation pipeline instead of Q's `h_s2`. That launch needs the trainer's aux trunk parameter pointers (zero-copy — same architectural contract as `trainer_params_ptr`). Added:
- 6× `u64` fields on `GpuExperienceCollector`: `aux_trunk_{w1,b1,w2,b2,w3,b3}_ptr` (init to 0).
- `exp_aux_trunk_forward_ops: AuxTrunkForwardOps` field, populated in the collector constructor via `AuxTrunkForwardOps::new(&stream)` on the **collector's stream** (not the trainer's) — same-stream contract per `pearl_no_host_branches_in_captured_graph`.
- Public setter `set_trainer_aux_trunk_param_ptrs(w1, b1, w2, b2, w3, b3)` — mirrors `set_trainer_params_ptr`'s shape.
- Trainer-side accessor `GpuDqnTrainer::aux_trunk_param_ptrs() -> (u64×6)` returning `self.aux_trunk_{w,b}{1,2,3}.raw_ptr()`. Pattern matches the existing `params_buf_ptr() / target_params_buf_ptr()` family.
- Setter wired at both `training_loop.rs` `set_trainer_params_ptr` call sites (pre-existing fused-ctx init + fused-ctx fold-boundary re-init); the new setter call passes the unpacked tuple from the trainer's accessor.
DEAD: ptrs are populated post-fused-ctx-init but no collector-side launch reads them in C.5a-fixup.
### Gap 3 — Graph-capture audit: aux_heads_backward IS INSIDE captured graph
**Result: INSIDE captured graph.** Verified by tracing the call chain:
- `aux_heads_backward` ([`gpu_dqn_trainer.rs:17663`](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)) is invoked from `launch_cublas_backward_to` ([`gpu_dqn_trainer.rs:29612`](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)), which is called by `launch_cublas_backward` ([line 29354](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)).
- `launch_cublas_backward` is called from `submit_forward_ops_main` ([line 27923](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)).
- `submit_forward_ops_main` runs inside the `forward` child graph captured by `capture_child_graph("forward", ...)` in [`fused_training.rs:3033-3037`](../crates/ml/src/trainers/dqn/fused_training.rs).
- The single CUDA stream `begin_capture` boundary in the trainer family is `fused_training.rs:2964` (`capture_child_graph` body); confirmed by `grep -nE "begin_capture" crates/ml/src/`.
**Implication for the existing aux_heads_backward.** The current implementation is `pub(crate) fn aux_heads_backward(&self, ...)` — entirely device-side: every operation is a kernel launch (`backward_next_bar`, `backward_regime`, 8× `param_grad_reduce` + SAXPY, 2× dh_s2 SAXPY). Zero host writes inside the function body — capture-safe by construction.
**Implication for C.5b's new aux trunk Adam launch.** The Adam launcher (`launch_aux_trunk_adam_update`, [`gpu_dqn_trainer.rs:9866`](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)) is also fully device-side (6× grad_norm partials → 1× finalize → 6× Adam update kernels). Capture-safe.
**Critical C.5b concern: the per-step host writes.** The C.5a allocations include three pinned host pointers needing per-step host writes before each Adam launch:
- `aux_trunk_grad_clip_pinned` — host writes ISV[`AUX_TRUNK_GRAD_CLIP_INDEX`].
- `aux_trunk_lr_pinned` — host writes ISV[`AUX_TRUNK_LR_INDEX`].
- `aux_trunk_adam_step` (host i32 mirror) → `aux_trunk_t_pinned`.
**These host writes CANNOT happen inside the captured `forward` child graph.** Two graph-capture-safe strategies are already established in the codebase:
1. **Pre-capture host write pattern.** `step_selectivity_adam` ([`gpu_dqn_trainer.rs:32169`](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)) demonstrates the pattern: increment host counter (`self.sel_adam_step += 1`), `unsafe { *self.sel_t_pinned = step_val }`, then issue kernel launches. **Note: `step_selectivity_adam` is not currently invoked from any captured-graph path** — it's an orphan, so it doesn't disprove the constraint. C.5b would need to issue the host writes outside the captured region (e.g., a pre-capture helper or in `process_epoch_boundary`) so the captured graph just records the device-side launches reading the pre-set pinned values.
2. **GPU-side step counter increment kernel** (PREFERRED for graph capture). The trainer already loads an `increment_step_counters` kernel ([line 22793](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)) plus a `submit_counters_ops` capture point ([line 22799](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs)) explicitly designed to bump all 8 main Adam step counters from inside the graph. **C.5b should add the aux trunk's `t_pinned` to the same `submit_counters_ops` chain so the step counter is incremented graph-side.** ISV-driven LR + clip values are already mapped-pinned with host-readable dev_ptrs; the host writes the LR/clip _before_ entering the captured region (cold-path per-epoch / per-fold), and the kernel reads `*lr_ptr` / `*clip_ptr` at execution time. This is the pattern the existing `lb_budget_*_dev_ptr` chain uses ([fused_training.rs:1940-1946](../crates/ml/src/trainers/dqn/fused_training.rs)) — capture records pointer reads, not values.
**C.5b wiring strategy (binding for C.5b's implementer):**
- Insert the new Adam launch inside the captured `forward` child graph, immediately after the existing `aux_heads_backward` SAXPY into `bw_d_h_s2`. Both are device-side kernel chains, both run on the same captured stream.
- Add the aux trunk's `t_pinned` to the `submit_counters_ops` GPU-side increment chain (or a sibling captured child) — DO NOT use host-side `aux_trunk_adam_step += 1` patterns inside the captured region.
- ISV[`AUX_TRUNK_LR_INDEX`] / ISV[`AUX_TRUNK_GRAD_CLIP_INDEX`] writes happen pre-capture (cold-path host writes to `aux_trunk_lr_pinned` / `aux_trunk_grad_clip_pinned`); the captured graph reads via the pre-cached `aux_trunk_lr_dev_ptr` / `aux_trunk_grad_clip_dev_ptr`.
- The `&self``&mut self` ripple on `aux_heads_backward` (gap 4) is therefore avoided IF C.5b uses the GPU-side counter increment path. If C.5b instead adopts the pre-capture host-write strategy, it must restructure the call site so the host writes occur before `capture_child_graph("forward", ...)` begins — which is non-trivial because the `forward` child is composed once at capture-time (line 3033) and its body is the same closure on every replay.
### Files modified (this commit)
- Modify: [`crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs`](../crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs) — +2 fields (`dh_aux1_pre_scratch`, `dh_aux2_pre_scratch`), +2 allocations adjacent to C.5a's site, +`aux_trunk_param_ptrs()` accessor.
- Modify: [`crates/ml/src/cuda_pipeline/gpu_experience_collector.rs`](../crates/ml/src/cuda_pipeline/gpu_experience_collector.rs) — +6× `u64` ptr fields + 1× `AuxTrunkForwardOps` field on `GpuExperienceCollector`, +1× orchestrator construction in collector ctor, +`set_trainer_aux_trunk_param_ptrs` setter.
- Modify: [`crates/ml/src/trainers/dqn/trainer/training_loop.rs`](../crates/ml/src/trainers/dqn/trainer/training_loop.rs) — +setter calls at the two existing `set_trainer_params_ptr` sites (~line 1703 / ~line 2317).
- Modify: this audit doc.
### Verification
- `SQLX_OFFLINE=true CUDA_COMPUTE_CAP=86 cargo check -p ml --tests --all-targets` — clean (only pre-existing warnings; no new errors).
- `SQLX_OFFLINE=true CUDA_COMPUTE_CAP=86 cargo test -p ml --test aux_trunk_oracle_tests --test sp14_oracle_tests --release -- --ignored --nocapture` — 12/12 pass (8 aux_trunk + 4 sp14, bit-identical to C.5a baseline; no regression — pure scaffolding).
- `grep -rn "set_trainer_aux_trunk_param_ptrs\|aux_trunk_param_ptrs\|dh_aux[12]_pre_scratch" crates/ml/src/` shows the new symbols defined and called at the exact 2 setter sites + 2 trainer-side allocations + 2 trainer-side struct-literal entries.
### Wire status — DEAD scaffolding (post-C.5a-fixup)
| Component | Status | Lit by C.5b? |
|-----------|--------|--------------|
| `dh_aux1_pre_scratch` / `dh_aux2_pre_scratch` (trainer) | allocated, no consumers | ✓ via `aux_trunk_backward.launch` |
| `aux_trunk_param_ptrs()` accessor (trainer) | defined, called from training_loop | ✓ result already populates the setter |
| `set_trainer_aux_trunk_param_ptrs` (collector) | called, fields populated, no readers | ✓ via collector-side `aux_trunk_forward.launch` |
| `aux_trunk_w{1,2,3}_ptr` / `b{1,2,3}_ptr` (collector) | populated, no readers | ✓ |
| `exp_aux_trunk_forward_ops: AuxTrunkForwardOps` (collector) | constructed, no `.launch()` calls | ✓ |
### Follow-up — C.5b is now scaffolding-free
With C.5a + C.5a-fixup providing all scaffolding (buffers, scratch, ptrs, accessor, setter, AuxTrunkForwardOps in collector), C.5b becomes a true atomic contract flip:
1. Param-rename `h_s2 → h_s2_aux` in the 4 aux_heads kernels.
2. Revert zero-fills at `aux_next_bar_backward:599-638` + `aux_regime_backward:757-790`.
3. Insert `aux_trunk_forward.launch(...)` post-`forward_online_f32` (collector) + post-encoder forward (trainer).
4. Insert `aux_trunk_backward.launch(...)` + `launch_aux_trunk_adam_update(...)` in the trainer's backward chain (graph-capture-safe per the audit above).
5. Add aux trunk `t_pinned` to `submit_counters_ops` chain (GPU-side step counter increment).
6. Pre-capture host write of ISV-driven `aux_trunk_lr_pinned` / `aux_trunk_grad_clip_pinned` (cold-path per-epoch).
7. Redirect `aux_heads_fwd` input pointer from Q's `h_s2` to `h_s2_aux`.