[Bug] Forward fastcache dataclass fields read only in a dead qd.static branch through every caller.#807
Conversation
…c branch through every caller. A dataclass field read only inside a compile-time-dead qd.static branch is still marked used under the callee's func id, which is shared across the callee's qd.static template instantiations, via the branch's live instantiation. The enforcing pass then forwards that field from every caller, but the discovery pass only copied a callee's used set into a caller at the instant of each recorded call. A caller whose call site is walked before the live-branch instantiation is discovered therefore never records the field, and the enforcing pass emits a forward the caller never bound - surfacing as an unbound name or a "Missing argument" argument-count mismatch depending on where the shortfall lands. Record each caller -> callee by-name forwarding as an edge during discovery and, once every callee used set is final, replay the edges to a fixpoint so each caller's used set is a superset of every field it forwards. This closes over multi-hop call chains and arbitrary dataclass nesting depth. Regression tests test_prune_used_parameters_fastcache_dead_static_branch and its _nested variant fail before this change on both arch configs and pass after.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f30b6f894b
ℹ️ 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".
|
Definitely needs Genesis benchmarks and unit tests on this. |
…llers sharing a flat name do not clobber.
Benchmarks? Why? |
…used cases instead of one parametrized function.
|
@codex review |
fastcache stuff tends to trash the benchmarks really often. I haven't looked at the code paths involved at all, but 9/10 of fastcache changes cause benchmark regressions. So let's check please. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01bba3855f
ℹ️ 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".
|
I got Opus 4.8 to review this, since fastcache stuff is incredibly sensitive. Its outputs are positive. See below. It had nothing critical to say. It left one minor comment: "The single-shot copy is now redundant. With the fixpoint replaying the same edges against a superset of the callee sets, the vars_to_unprune accumulation in record_after_call (lines 80, 98-99, 112-115) no longer affects the final result. I would not rip it out though - keeping it minimizes contact area and lowers risk, consistent with AGENTS.md. Worth a one-line comment noting it's now belt-and-suspenders, if anything." Perhaps we can add a FIXME as agent suggests, but not remove vars_to_unprune for now, for the reasons AI gives. (I have not looked at the code (though I once wrote it 😅, I remember creating a variable with that name ), just running with AI's suggestion here). ==== start of AI output === What it actually isDespite the 305-file diff vs your local The bug and the fixThe root cause is real and subtly stated correctly: The fix records every caller -> callee by-name forwarding as an edge and replays them to a fixpoint ( Correctness - looks sound
Test quality - strongThe 3x loop over a shared Minor observations (none blocking)
Overall: tight, well-reasoned fix with a genuinely good regression test. The commit message and inline comments are unusually clear about the mechanism. |
Fastcache is not used for performance mode, and we don't care much about perf for ndarray, useless it is completely trash of course. |
|
Opus 4.8 states:
AI output: |
…s to one callee do not clobber.
…es so the edge name is never None.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
This branch: main branch: |
|
Interpretation
Note: the std comes from the official campaign's baseline runs available here. |
2 participants
Brief Summary
fastcache=Truefails to compile a kernel when a@qd.func/kernel dataclass field is read only inside a compile-time-deadqd.staticbranch and the enclosing struct is forwarded through more than one caller path. It surfaces as either an unbound__qd_...name or aQuadrantsSyntaxError: Missing argument ..., depending on which call the un-forwarded field lands on. Plain nested dataclasses (any depth) already work; the trigger is specifically the dead-static-branch field combined with the discovery-order dependence below.Walkthrough
Root cause: a field read only inside a dead
qd.staticbranch is still marked used under the callee's func id (shared across the callee'sqd.statictemplate instantiations) via the branch's live instantiation. The enforcing pass therefore forwards that field from every caller. But the discovery pass copied a callee's used set into a caller only at the instant of each recorded call, so a caller whose call site is walked before the live-branch instantiation is discovered never records the field. The enforcing pass then emits a forward the caller never bound.Fix (
_pruning.py,kernel.py): record each caller -> callee by-name forwarding as an edge during discovery, and once every callee used set is final (after pass 0, before the parent-prefix reduction) replay the edges to a fixpoint. Each caller's used set thus becomes a superset of every field it forwards. This closes over multi-hop call chains and arbitrary dataclass nesting depth.The enforcing pass also consults a recorded map from each caller argument name to the callee parameter it binds, to decide which forwarded arguments to keep. Recording that map unconditionally - needed so a dead-branch call site still forwards the field - exposed a second issue: the map was shared per callee, so two calls binding the same flat name to different callee slots (used in one, unused in the other) overwrote each other, and the enforcing pass then pruned the field the first call actually needs. The map is now keyed per call site by
(caller_func_id, callee_func_id, node.lineno, node.col_offset)(_pruning.py,call_transformer.py); the source position is stable across the two passes, so each call site records its own map, whether the two calls sit in different callers or repeat inside one caller.All of the above is compile-time pruning logic in
materialize; the generated kernel and its launch path are unchanged.Tests
Regression tests in
test_py_dataclass.py, each failing on the unfixed tree on both arch configs and passing after:test_prune_used_parameters_fastcache_dead_static_branch- two caller paths forward the struct to a callee that reads the field only in a deadqd.staticbranch, with the dead-branch caller walked first.test_prune_used_parameters_fastcache_dead_static_branch_nested- same, but the forwarded field lives three dataclass levels deep.test_prune_used_parameters_fastcache_forward_same_name_swapped_slots- two callers forward the same flat name into swapped callee slots (used in one, unused in the other), pinning the per-call-site map keying across callers.test_prune_used_parameters_fastcache_forward_same_name_swapped_slots_same_caller- the same swap across two call sites inside a single caller.Full
test_py_dataclass.py+fast_caching/+test_kernel_impl_dataclass.pysuites: 285 passed, 6 xfailed, 0 regressions.