Skip to content

[Bug] Fix SPIR-V dominance of the adstack heap stride loads.#806

Merged
duburcqa merged 1 commit into
mainfrom
duburcqa/fix_spirv_adstack_stride_dominance
Jul 22, 2026
Merged

[Bug] Fix SPIR-V dominance of the adstack heap stride loads.#806
duburcqa merged 1 commit into
mainfrom
duburcqa/fix_spirv_adstack_stride_dominance

Conversation

@duburcqa

@duburcqa duburcqa commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Issue: #805

The reverse-mode collapse reported in #805 is not an Apple compiler uniformity bug: get_ad_stack_metadata_stride_(float|int) memoize the stride OpLoad at their first call site, so when the first f32 heap access of a task sits inside one arm of a runtime if, every access in the sibling arm reuses an SSA id that does not dominate it. spirv-val rejects the module (section 2.16), but SPIRV-Cross does not validate and emits the reused id as a plain uninitialized MSL variable, so the per-thread heap base row_id * stride multiplies a correct per-lane row_id by a garbage stride (typically 0) and every thread of the dispatch races on the same heap row. The fix seeds both memoized stride loads at the task entry block (preload_ad_stack_metadata_strides, called from each generate_*_kernel, gated on the pre-pass adstack count so adstack-free tasks do not bind the metadata buffer).

Brief Summary

copilot:summary

Walkthrough

copilot:walkthrough

@duburcqa duburcqa changed the title [Bug] Seed the adstack heap stride loads at task entry so their SPIR-V ids dominate sibling branch push sites. [Bug] Fix SPIR-V dominance of the adstack heap stride loads. Jul 21, 2026
Base automatically changed from duburcqa/fix_adstack_tensor_loop_carried_grad to main July 21, 2026 19:08
@duburcqa
duburcqa force-pushed the duburcqa/fix_spirv_adstack_stride_dominance branch from ea0f597 to fe617bf Compare July 21, 2026 19:22
@duburcqa

duburcqa commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Genesis PR #2842 (29e2c22e5) x quadrants PR #806 (fe617bffe), with the Genesis-side Metal gradient skip lifted.

MacBook Pro: pytest tests/grad -n 8 --backend cpu

================= 58 passed, 12 warnings in 233.71s (0:03:53) ==================

MacBook Pro: pytest tests/grad -n 8 --backend gpu

============ 56 passed, 2 skipped, 12 warnings in 242.28s (0:04:02) ============

The 2 skips are the fp64 arms, which Metal does not support.

@hughperkins

Copy link
Copy Markdown
Collaborator

@codex review

@hughperkins

Copy link
Copy Markdown
Collaborator
  • skimming the code looks reasonable to me: relatively small well-defined changes, albeit to general spirv file ✅
  • lets wait for CI agent checks to run ⌛

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Unknown error
ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@github-actions

Copy link
Copy Markdown

@hughperkins

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: fe617bffe8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@duburcqa

Copy link
Copy Markdown
Contributor Author

Production CI: GS_ENABLE_NDARRAY=1 pytest -v -ra --dev --backend gpu --forked tests/grad
============== 58 passed, 17 warnings in 278.54s (0:04:38) ===========

@github-actions

Copy link
Copy Markdown

@hughperkins

Copy link
Copy Markdown
Collaborator

ok to merge

@duburcqa
duburcqa merged commit c98cbac into main Jul 22, 2026
67 of 68 checks passed
@duburcqa
duburcqa deleted the duburcqa/fix_spirv_adstack_stride_dominance branch July 22, 2026 12:48
@hughperkins

Copy link
Copy Markdown
Collaborator

Note: no need for Genesis benchmarks, because spir-v specific, and benchmarks don't exercise spir-v.

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