fix(ir): forbid tile.move in-place reuse (root fix for #2100), revert #2100 workarounds - #2106
fix(ir): forbid tile.move in-place reuse (root fix for #2100), revert #2100 workarounds#2106YunjiQin wants to merge 3 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Changestile.move aliasing
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 12a8fc89e3
ℹ️ 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".
12a8fc8 to
8f00ac1
Compare
YunjiQin
changed the title
8f00ac1 to
0560adf
Compare
…c-only reuse gate Revert the two workarounds from hw-native-sys#2100 (a90f158): the codegen same_layout guard on tile.move elision and the AreVecNdNzCompatible Vec-only ND<->NZ reuse gate in MemoryReuse. Both are superseded by marking tile.move .not_inplace_safe(): forbidding the move output from reusing its input buffer prevents the same-address ND->NZ colocation at its source, so neither the layout-aware elision special-case nor the per-layout reuse gate is needed.
Mark tile.move .not_inplace_safe() so MemoryReuse never lands the move output on its input's buffer. The TMOV intrinsic cannot execute with src == dst (a same-address move is not a legal instruction, only a no-op), so a same-space layout-changing move (e.g. the A5 V->C ND->NZ fractal adapt) colocated with its source would be seen by codegen as a same-address no-op and wrongly elided, dropping the layout adapt. Add a regression test asserting a same-space tile.move output gets a buffer distinct from its input.
With tile.move marked .not_inplace_safe(), MemoryReuse can no longer place a move output on its input buffer, so under baked addresses src and dst are always distinct — the same-space same-address elision can never fire. Remove that dead baked-address branch (and its now-unused includes); tile.move always emits pto.tmov on the baked-address path. The PtoAS branch is unchanged: under memory_planner=PtoAS the reuse gate is skipped and a YieldFixup loop-carry write-back can still collapse onto one tile_buf handle, so the handle-based elision stays to avoid emitting an illegal same-handle pto.tmov.
1 participant
Summary
Root-cause fix for the A5 V→C miscompile that #2100 worked around, plus removal of the now-redundant workarounds.
The
TMOVintrinsic cannot execute withsrc == dst— a same-address move is not a legal instruction, only a no-op.tile.movewas defaulting to in-place-safe, which letMemoryReusecolocate a same-space layout-changing move (the A5 V→C ND→NZ fractal adapt) with its source at one Vec address. Codegen then saw matching src/dst addresses and elided thepto.tmovas a no-op, silently dropping the layout adapt (RowMajorTPUSHvs ColMajorTPOP) → silent numerical FAIL.Marking
tile.move.not_inplace_safe()forbids that colocation at the op level, sosrcanddstalways get distinct buffers. This removes the same-address precondition entirely, rather than tolerating it and special-casing the symptom downstream.Commits
revert(codegen/ir): drop #2100 layout-aware elide and Vec-only reuse gate— reverta90f1584(fix(codegen/ir): keep layout-aware tmov elide; Vec-only ND↔NZ reuse gate #2100): the codegensame_layoutguard ontile.moveelision and theAreVecNdNzCompatibleVec-only ND↔NZ reuse gate inMemoryReuse. Both are workarounds superseded by the root fix below.fix(ir): forbid tile.move output from reusing its input buffer— marktile.move.not_inplace_safe(); add a regression test asserting a same-spacetile.moveoutput gets a buffer distinct from its input.refactor(codegen): drop same-address tmov elision for tile.move— with the move output never on its input buffer, the same-space same-address elision can never fire under baked addresses; remove that dead branch (and now-unused includes).tile.movealways emitspto.tmovon the baked-address path.Why this supersedes #2100
pto.tmovwhenblayout/slayout/fractaldiffer and (b) added a Vec-only ND↔NZ reuse gate. Both special-case the symptom.tile.movenever reaches codegen.No collateral cost
Through the current Default pipeline, legitimate same-space same-layout no-op moves are not produced: loop-carry buffers are converged onto one base by
MaterializeSemanticAliases/AlignLoopCarriesToInitvia in-place retyping, not by inserting a move. Empirically, the same-address codegen elision is never hit by anytests/ut/codegentest even onmain(0 hits across the suite). Cross-space moves (Mat→Left, etc.) live in different base-groups and are unaffected. The narrownot_inplace_safeflag does not touch the reuse packer's coalescing of unrelated lifetime-disjoint L0A tiles, so the split-KLeftcapacity path is unchanged.PtoAS path is preserved: under
memory_planner=PtoASthe reuse gate is skipped and aYieldFixuploop-carry write-back can still collapse onto onetile_bufhandle, so the handle-based elision (!EmitTileAddr()branch) is intentionally kept to avoid emitting an illegal same-handlepto.tmov.Testing
tests/ut/ir/transforms/test_memory_reuse.py+tests/ut/codegen/test_pto_codegen_ops.py— 155 passed (incl. new test and the acc→acc regressions)tests/ut/ir/transforms/+tests/ut/codegen/— 2850 passed (no reuse / L0 capacity regression)Related
Root-cause fix for the issue addressed by #2100; reverts #2100's workarounds.