Fix TPut release fence before TNotify#873
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 refactors the TNotify release analysis to track pipe drains and DDR-domain release fences independently using a new TNotifyReleaseState struct instead of a simple bitmask. This ensures that internal stores from operations like pto::TPutOp are properly drained and made DDR-visible before a notification signal is published, even if a pipe barrier is already present. The review feedback suggests improving the precision of the analysis in multi-block regions by operating on specific Regions rather than the parent Operation to avoid overly conservative barrier insertions, and adding a check to skip external function declarations in annotateTNotifyRelease to prevent potential crashes when calling getBody() on functions without a body.
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.
| TNotifyReleaseState regionState = collectTNotifyReleaseState(&op); | ||
| TNotifyReleaseState nestedNotifyState = regionEntryState; | ||
| nestedNotifyState.merge(nestedLoopCarriedState); | ||
| nestedNotifyState.merge(regionState); | ||
| markNestedTNotifyWithState(&op, nestedNotifyState); |
There was a problem hiding this comment.
In multi-block regions, calling collectTNotifyReleaseState(&op) and markNestedTNotifyWithState(&op, ...) on the parent operation op instead of the specific region causes the analysis to be overly conservative. If an operation has multiple regions (such as scf::IfOp with then and else branches), a multi-block structure in one branch will conservatively collect and apply the release state to both branches, potentially leading to redundant barriers/fences.\n\nWe can improve precision by overloading collectTNotifyReleaseState and markNestedTNotifyWithState to operate directly on Region & instead of Operation *:\n\ncpp\nstatic TNotifyReleaseState collectTNotifyReleaseState(Region ®ion) {\n TNotifyReleaseState state;\n for (Block &block : region)\n for (Operation &nested : block)\n state.merge(collectTNotifyReleaseState(&nested));\n return state;\n}\n\nstatic void markNestedTNotifyWithState(Region ®ion,\n const TNotifyReleaseState &state) {\n region.walk(\n [&](pto::TNotifyOp notify) { setTNotifyReleaseAttrs(notify, state); });\n}\n
TNotifyReleaseState regionState = collectTNotifyReleaseState(region);\n TNotifyReleaseState nestedNotifyState = regionEntryState;\n nestedNotifyState.merge(nestedLoopCarriedState);\n nestedNotifyState.merge(regionState);\n markNestedTNotifyWithState(region, nestedNotifyState);| for (auto func : module.getOps<func::FuncOp>()) { | ||
| if (func.getBody().hasOneBlock()) { |
There was a problem hiding this comment.
If the module contains external function declarations (which have empty bodies), calling func.getBody().hasOneBlock() might be unsafe or lead to unnecessary processing. It is a standard best practice in MLIR passes to explicitly skip function declarations early.
for (auto func : module.getOps<func::FuncOp>()) {\n if (func.isDeclaration())\n continue;\n if (func.getBody().hasOneBlock()) {
TaoTao-real
force-pushed
the
codex/fix-issue872-tput-tnotify-release
branch
5 times, most recently
from
6d92cfc to
ad97aef
Compare
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
TaoTao-real
force-pushed
the
codex/fix-issue872-tput-tnotify-release
branch
2 times, most recently
from
bcf801c to
f96297f
Compare
|
Update for PR1 hardening:
Local validation:
|
|
Follow-up review update:
Validation:
Commit: |
TaoTao-real
force-pushed
the
codex/fix-issue872-tput-tnotify-release
branch
from
3222bc0 to
e0c24e9
Compare
|
PyPTO integration note for this PR:
The CI fix commit also exposes |
|
Follow-up on the PyPTO-facing API shape: I added Python AttrBuilder registration for the memory-consistency enum attrs, so PyPTO no longer needs to build Preferred PyPTO lowering-side form is now: pto.TPutOp(...)
pto.FenceReleaseOp(pto.FenceScope.DDR)
pto.TNotifyOp(...)For cacheable GM scalar paths, the same enum-direct style works: pto.CmoCleanOp(pto.AddressSpace.GM)
pto.FenceReleaseOp(pto.FenceScope.DDR)
...
pto.CmoInvalidateOp(pto.AddressSpace.GM)The generated IR is unchanged ( |
|
Non-blocking follow-up notes for memory-consistency precision:
These are optimization/precision items, not current correctness blockers for the straight-line issue paths covered by this PR. I will add lit coverage for the current conservative behavior and the newly supported scalar release sequence. |
|
/run A3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A3 板测完成（有跳过）
|
|
/run A3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
# Conflicts: # lib/PTO/Transforms/VPTOCANN900LLVMEmitter.cpp # lib/PTO/Transforms/VPTOLLVMEmitter.cpp # tools/ptobc/src/mlir_encode.cpp
A3 板测完成（有跳过）
|
|
/run A3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A3 板测完成（有跳过）
|
2 participants
Design Document
docs/designs/ptoas-memory-consistency-design.mdrecords the memory-consistency contract, PyPTO emission patterns, EmitC lowering support, and current VPTO fail-fast boundary.Summary
pto-memory-consistencyModule pass that validates release/acquire memory-consistency contracts before backend lowering.pto.cmo.clean all #pto.address_space<gm>: GM cache clean.pto.cmo.invalidate all #pto.address_space<gm>: GM cache invalidate.pto.fence.release #pto.fence_scope<ddr>: DDR-domain release fence.pto.fence.acquire #pto.fence_scope<ddr>: DDR-domain acquire fence.dcci((__gm__ void*)0, ENTIRE_DATA_CACHE, CACHELINE_OUT)dcci((__gm__ void*)0, ENTIRE_DATA_CACHE)dsb(DSB_DDR)dcciordsb; missing or misordered explicit CMO/fence operations are reported as compile-time errors.pto.fence.release #pto.fence_scope<ddr>follows pending GM writes on MTE3 or FIX, PTOAS inserts the matchingpto.barrierimmediately before the release fence, so EmitC producespipe_barrier(<producer pipe>); dsb(DSB_DDR); TNOTIFY.SyncMacroModelis used to recognize macro-op MTE3 phases, sopto.comm.tputand other comm macro GM-store phases are validated beforeTNotify.Fixes #872.
PyPTO Contract
pto.barrier #pto.pipe<PIPE_MTE3>orpto.barrier #pto.pipe<PIPE_FIX>forTStore,TStoreFP, orTPUTpublish paths. It should emit the semantic boundary:pto.tstoreorpto.comm.tputpto.fence.release #pto.fence_scope<ddr>pto.comm.tnotifypto.cmo.clean all #pto.address_space<gm>beforepto.fence.release #pto.fence_scope<ddr>.TWaitor successfulTTest, PyPTO needs to emitpto.cmo.invalidate all #pto.address_space<gm>beforepto.load_scalar.TNotify,TWait, orTTestmust be inlined beforepto-memory-consistency.pto.entrylauncher calls to kernels are allowed.Covered Scenarios
TNotify: requires explicitpto.fence.release #pto.fence_scope<ddr>; PTOAS auto-inserts MTE3 pipe drain before the fenceTNotify, such as ACCTStoreorTStoreFP: requires explicitpto.fence.release #pto.fence_scope<ddr>; PTOAS auto-inserts FIX pipe drain before the fenceTNotify: still emits onlypipe_barrier(PIPE_MTE2), no DDR fenceTNotify: requires explicitpto.fence.release #pto.fence_scope<ddr>and does not duplicate pipe drainsTPUT -> TNotify:TPUTis recognized through macro modeling and must be followed by explicit DDR release fence before signal publish; PTOAS supplies the MTE3 drainTBroadcast -> TNotify: guards the genericSyncMacroModelMTE3 phase path, not just aTPUTspecial caseTNotify: requires explicit clean plus DDR release fence before signal publishTWait/TTest -> load_scalar: requires explicit acquire invalidate before scalar GM payload readmemory_consistency_invalid.ptopto.cmo.*andpto.fence.*with clear unsupported-lowering diagnostics until VPTO/Bisheng exposes the DSB/DCCI ABI PTOAS should callTests
git diff --check hw-native-sys/main...HEAD/Users/lishengtao/Documents/PTO/llvm-workspace/llvm-project-21/build-sharedninja -C build-pr873-ci obj.PTOTransformsninja -C build-pr873-ci ptoascheck-pto: 773/779 passed; the remaining 6 fail in TileLang template import because the local CMake used Python 3.9 and current TileLang templates use Python 3.10 union type syntax such asstr | None. These failures are unrelated to this PR and are expected to be covered by GitHub CI's Python 3.11 environment.