Remove automatic memory consistency pass#953
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the automatic pto-memory-consistency analysis pass from the PTOAS pipeline to address compile-time performance degradation in complex control flows (issue #950). The memory consistency model transitions back to an explicit IR contract where users or PyPTO must manually insert cache maintenance operations (CMO), visibility fences, and pipe barriers. The design documentation, operator definitions, tests, and compiler pipeline have been updated accordingly to reflect this change. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
zhangstevenunity
left a comment
zhangstevenunity
left a comment
There was a problem hiding this comment.
COMMENT review. This is a clean, well-scoped removal that correctly targets the root cause of #950 (the unbounded state merge in annotateTNotifyRelease that doubled pendingAccesses per single-block scf.if, going ~2^k over the ~80 sequential cursor ifs in the CSA repro).
Verified on my side:
- Removal is complete and dangling-reference-free. Every
createPTOMemoryConsistencyPassreference, thePasses.td/Passes.hdecls, the CMake source entry, and theptoas.cpppipeline call live only in files this PR deletes/edits. Nothing else calls the factory. - The internal attr constants (
kCmoCacheInvalidSkipLowering*,kTNotifyDrain*) are defined inMemoryConsistencyAttrs.h, not in the deleted.cpp, so the dormant EmitC hooks that still read them still link. PTOFenceToEmitC<...>is instantiated only forFenceBarrierAllOp(PTOToEmitC.cpp:14304), so the new conservative drain applies only topto.fence.barrier_all.- The conservative fence drain is safe-direction (
pipe_barrieris a wait, no deadlock) and correctly ordered (drains beforedsb). For the explicit-IR path it preserves #711/#872: an explicitfence.barrier_allbeforetnotifystill drains MTE2/MTE3/FIX + DSB, which is >= the precise drain the old pass used to insert. - Untouched fence test
comm_p2p_emitc.ptouses loose ordered checks and is not broken; the Python binding testmemory_consistency_bindings.pyonly exercises the kept public CMO/Fence ops and stays green; docs and ODS descriptions are updated to match.
The residual risks below are consequences of dropping a correctness/diagnostic pass rather than bugs in this PR, and are largely covered by the Risk/Rollback section. Two are worth calling out explicitly (inline). Not a blocker.
| op.getScope().getScope() != pto::FenceScope::All) | ||
| return rewriter.notifyMatchFailure(op, "unsupported fence scope"); | ||
|
|
||
| emitConservativeGmFencePipeDrains(rewriter, op.getLoc()); |
There was a problem hiding this comment.
Correctness of this conservative drain looks right and safe-direction: pipe_barrier is a wait (no deadlock risk) and the drains-before-dsb order is correct, so an explicit fence.barrier_all before tnotify preserves the #711/#872 release ordering the removed pass used to insert.
One asymmetry worth flagging: the release side keeps a compensating mechanism here (the fence auto-drains MTE2/MTE3/FIX), but the acquire side gets nothing. The removed pass used to diagnose a cacheable GM load_scalar after twait/ttest that lacked a cmo.cacheinvalid; there is no lowering-level fallback for that. After this PR a consumer doing twait -> cacheable GM load with no explicit cmo.cacheinvalid silently reads stale cached data, with no error and no auto-insertion. Since the release path is compensated and the acquire path is not, please confirm that asymmetry is intended.
Minor, non-blocking: MTE2 is a GM->local read pipe and FIX is cube-only, so both are over-drained on pure-vector gm fences. Fine as a documented conservative choice.
| // TNotify release requires the producer side to drain local producer pipes | ||
| // before the GM fence and before pto.comm.tnotify. PTOAS no longer runs the | ||
| // historical MemoryConsistency analysis pass, so the explicit GM fence lowering | ||
| // conservatively drains MTE2, MTE3, and FIX before dsb(DSB_DDR). |
There was a problem hiding this comment.
After the rewrite this test only proves the conditional guarantee: "if the producer writes an explicit fence, the fence lowers to MTE2/MTE3/FIX drains + DSB." The original #711 failure mode -- a bare MTE3 tstore -> tnotify with no fence -- now emits no drain and no diagnostic, and no test covers that negative case. The test is still valuable for pinning the new fence lowering, but it no longer guards against #711 recurring in producer-generated IR. Consider adding a negative/diagnostic test, or renaming, so the weaker contract is explicit.
| } | ||
|
|
||
| func.func @single_line_twait_load_scalar_acquire( | ||
| func.func @scalar_acquire_sequence( |
There was a problem hiding this comment.
This acquire sequence (twait -> cmo.cacheinvalid -> cacheable load) is now the only thing standing between a correct read and a stale one, and it is entirely producer-authored: if a producer omits the cmo.cacheinvalid, the load silently returns stale cache with no diagnostic (the removed pass used to error here). Unlike the release path, there is no lowering-level safety net.
The whole "producer responsibility" model hinges on in-tree producers (PyPTO / TileLang templates) already emitting these explicit CMO/fence ops around every signal/payload handoff. This PR validates hand-written lit IR and the #950 compile-time repro, but not an end-to-end produced kernel that previously relied on the auto pass. Worth confirming no production template regresses before merge.
A5 板测成功
|
A3 板测失败
失败用例
|
A3 板测失败详情：PR #953comm_p2p_binding_variants
comm_p2p
|
3 participants
Summary
pto-memory-consistencyanalysis pass from registration, build files, and the defaultptoaspipeline.pto.cmo.cacheinvalid,pto.fence.barrier_all) and EmitC lowering.Motivation
scf.if, child-region state already carried entry state, and the parent merged it back by appending vectors. On the perf: ptoas 0.50 >160x compile slowdown on csa_slots_build_valid_qk_plan kernel vs 0.48 #950 repro this produced near-exponential growth across ~80 consecutivescf.ifregions.Design
PTOMemoryConsistency.cpp, its TableGen pass definition, factory declaration, CMake entry, and default pipeline invocation.pto.barrier+pto.fence.barrier_alllowering checks.Testing
ninja -C build ptoasOK.build/tools/ptoas/ptoas /tmp/csa_slots_build_valid_qk_plan.pto -o /tmp/csa_out.cpp --enable-insert-sync --pto-level=level3 --pto-arch a3OK,real 0.87.comm_p2p_emitccmo_cacheinvalid_single_line_invalidissue711_tnotify_mte_drainissue872_tput_tnotify_releasetnotify_release_local_opssignal_payload_cache_consistencymemory_consistency_cmo_unsupportedmemory_consistency_fence_unsupportedgit diff --checkOK.ModuleNotFoundError: No module named 'mlir.dialects'), not from this change path.Risk / Rollback