Skip to content

[Bug] Fix the reverse-mode gradient of a loop-carried vector rebuilt across a dynamic loop.#799

Merged
duburcqa merged 6 commits into
mainfrom
duburcqa/fix_adstack_tensor_loop_carried_grad
Jul 21, 2026
Merged

[Bug] Fix the reverse-mode gradient of a loop-carried vector rebuilt across a dynamic loop.#799
duburcqa merged 6 commits into
mainfrom
duburcqa/fix_adstack_tensor_loop_carried_grad

Conversation

@duburcqa

Copy link
Copy Markdown
Contributor

Issue: #797

Stacked on #796 (the scalarize size_expr propagation): promoting a loop-carried tensor to an adstack relies on that fix to size the scalar component stacks. Review/merge #796 first; this PR's base will retarget to main once #796 lands.

Brief Summary

copilot:summary

Walkthrough

copilot:walkthrough

duburcqa added 2 commits July 20, 2026 15:37
`determine_ad_stack_size` runs before the scalarize pass. For an adaptive tensor-typed
adstack it records a symbolic `size_expr` and leaves `max_size` at the seed value of 1
for the runtime to overwrite from that tree. When scalarize splits the tensor stack into
one scalar `AdStackAllocaStmt` per component it copied `max_size` but dropped `size_expr`,
so the launcher found no tree and sized the component heap to the seed. A scalar component
stack pushed inside a runtime-bounded loop then overflowed as soon as the loop ran more
than once - a loud assertion on SPIR-V, silently corrupted gradients on a `__debug__`-
disabled build.

Clone `size_expr` onto every scalar component. The components are pushed and popped in
lockstep with the tensor stack, so they share its depth exactly.

Fixes #793.
@hughperkins

Copy link
Copy Markdown
Collaborator
  • scoped to adstack only => no need for genesis benchmarks
  • probably good to run genesis unit tests though I think? (since they do run some adstack things I think? or no?)
  • no user-facing doc changes required ✅

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

duburcqa added 2 commits July 20, 2026 21:04
…across a dynamic loop.

A multi-component `qd.Vector` carried across a field-bounded loop and rebuilt from its own
previous components produced a wrong reverse-mode gradient (the second-order term scaled by
a spurious extra factor), silently on `__debug__`-disabled builds. The scalar-valued
analogue was already correct. Two defects, both specific to the tensor component read:

1. The adstack judge (`AdStackAllocaJudger`) decides a loop-carried local needs an adstack
   via the "loaded AND stored inside a dynamic loop" rule, keyed on `local_loaded_`. That
   flag was set only for a direct `LocalLoad` of the alloca. A tensor local is read
   component-wise through a `MatrixPtr`, so the flag never fired and the loop-carried tensor
   stayed a single overwrite-each-iteration slot with no per-iteration history. Match the
   `MatrixPtr`-rooted load so tensor locals promote to an adstack like scalars do.

2. `ReplaceLocalVarWithStacks` subscripts a stack-backed tensor top by materializing it into
   a fresh plain alloca (so the forward store-to-load walker can trace it). That alloca is
   not recomputable, so `BackupSSA` spilled the component read to a single
   overwrite-each-iteration slot and every reverse iteration read the last forward
   iteration's component. Recognize this materialization shape and reconstruct the component
   in reverse by cloning the recomputable full-tensor top per iteration and re-subscripting.

Fixes #797.
@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

@duburcqa

duburcqa commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

MacBook Pro: pytest tests/core/test_grad.py -n 8 --backend cpu
============ 10 passed, 2 skipped, 6 warnings in 138.96s (0:02:18) =============
MacBook Pro: pytest tests/core/test_grad.py -n 8 --backend gpu
============ 10 passed, 2 skipped, 6 warnings in 137.63s (0:02:17) =============

@duburcqa

Copy link
Copy Markdown
Contributor Author

Production CI: GS_ENABLE_NDARRAY=1 pytest -v -ra --dev --backend gpu --forked tests
====== 867 passed, 9 skipped, 7 xfailed, 65 warnings in 1334.79s (0:22:14) =====

@duburcqa

duburcqa commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Genesis PR #2842 (94f257277) x quadrants PR #799 (d42a9ab)

MacBook Pro: pytest tests/core/test_grad.py tests/core/test_grad_fd.py tests/core/test_grad_optim.py tests/core/test_grad_utils.py -n 8 --backend cpu
=========== 220 passed, 132 skipped, 6 warnings in 259.85s (0:04:19) ===========
MacBook Pro: pytest tests/core/test_grad.py tests/core/test_grad_fd.py tests/core/test_grad_optim.py tests/core/test_grad_utils.py -n 8 --backend gpu
========== 220 passed, 132 skipped, 6 warnings in 230.17s (0:03:50) ===========
Production CI: GS_ENABLE_NDARRAY=1 pytest -v -ra --dev --backend gpu -n 16 --forked tests/core/test_grad*
======== 352 passed, 17 warnings in 957.55s (0:15:57) =========

@hughperkins

Copy link
Copy Markdown
Collaborator

ok to merge

Base automatically changed from duburcqa/fix_adstack_scalarize_size_expr to main July 21, 2026 13:32
@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

@duburcqa
duburcqa merged commit 22b7b19 into main Jul 21, 2026
58 checks passed
@duburcqa
duburcqa deleted the duburcqa/fix_adstack_tensor_loop_carried_grad branch July 21, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants