Reland InsertCommFence on ptoas 0.52 and unify the cacheinvalid codegen path - #2168
Open
luohuan19 wants to merge 9 commits into
Open
Reland InsertCommFence on ptoas 0.52 and unify the cacheinvalid codegen path#2168luohuan19 wants to merge 9 commits into
luohuan19 wants to merge 9 commits into
Conversation
…epSeek-V4-Flash moe to CI job
PTOAS v0.51 reorganised the release tarball: `<root>/ptoas` changed from a shell launcher into a Python package *directory*, leaving `<root>/bin/ptoas` as the only executable. Every codegen call resolved `$PTOAS_ROOT/ptoas` and so failed with "does not exist or is not executable", taking down the system-tests, system-tests-direct, system-tests-a5sim, dist-system-tests and pypto-lib-model jobs. Probe `<root>/ptoas` first and fall back to `<root>/bin/ptoas`. The order matters and is not cosmetic: up to v0.50 the top-level entry is a launcher that exports LD_LIBRARY_PATH=<root>/lib before exec'ing the bare binary, which carries no RUNPATH and otherwise dies on its bundled MLIR shared objects. From v0.51 the top-level entry is a directory, so the probe falls through to the now self-sufficient bin/ptoas. Launcher-first is correct on both layouts; bin/ptoas-first silently breaks every pre-v0.51 toolchain. The same discovery logic was duplicated in three modules, two of which degrade silently rather than raising: jit's `_ptoas_available()` flips `skip_ptoas` on, and `pto_rebuild` prints a skip line and rebuilds the stale kernel cpp. Both would have gone quiet under v0.51, so the probe is shared rather than patched at the one site CI happens to exercise. Also drop a trailing-whitespace-only line in the new pypto-lib CI steps that failed the pre-commit hook.
`pypto-lib-model` already runs `models/deepseek/v4-flash/prefill_attention_csa.py`, so the step added here was a byte-identical second copy: the example compiled and ran twice and burned an extra device allocation for no added coverage. The `moe` step is genuinely new and stays.
Placed with the other decode_attention_* steps in `pypto-lib-model`. This is the step the branch was meant to add: the earlier revision added a second `prefill_attention_csa`, which the job already ran.
Sharing the ptoas probe removed the module-level `_ptoas_binary`, which `tests/ut/runtime/test_pto_rebuild.py` monkeypatches in 15 tests — they all died with AttributeError. Bind the shared helper to that name instead of importing it under a new one, so the seam the tests patch still exists.
Both sha256 values verified against the release asset digests of hw-native-sys/PTOAS v0.52 (ptoas-bin-aarch64.tar.gz / ptoas-bin-x86_64.tar.gz). The tarball layout probe added for v0.51 (python/pypto/backend/ _ptoas_locate.py) is version-agnostic — it tests each candidate for being an executable file rather than keying off the release — so no discovery change is needed for v0.52.
This reverts d64380c (revert of hw-native-sys#2076), restoring the InsertCommFence pass, its Default/DebugTile pipeline registration, bindings, stubs, the CommRemoteOffset inlining, the distributed remote-op codegen changes and docs 43-insert_comm_fence.md. d64380c backed the pass out because ptoas 0.50/0.51 could not lower the publish-side region cacheinvalid: 0.50 parsed `pto.cmo.cacheinvalid <partition_tensor_view> single_cache_line` and emitted no call at all, and 0.51 emitted PTOAS__DCCI_SINGLE_CACHE_LINE(<GlobalTensor>), whose body casts to `__gm__ void*` — a conversion GlobalTensor does not have — so every kernel carrying the op failed to compile. hw-native-sys/PTOAS#1001 fixes this by adding a GlobalTensor overload that takes the address via tensor.data(), and shipped in v0.52, which this branch already pins. toolchain/versions.env is kept at v0.52; the revert's restore of the v0.50 pin is intentionally dropped. The pass-doc-ordering table keeps entry 42 and restores entry 43 as the last pass, matching d64380c's non-mechanical handling of that file.
The scalar-write special case emitted a bare pointer operand: %0 = pto.addptr %arg, %off : !pto.ptr<f32> -> !pto.ptr<f32> pto.cmo.cacheinvalid %0 single_cache_line ptoas rejects that outright. Measured against 0.52: | operand | result | | ----------------------------------- | ------------------------------- | | !pto.ptr, no type annotation | parse error: expected ':' | | !pto.ptr, with type annotation | lowering: "addptr must feed | | | make_tensor_view, ..." | | !pto.tensor_view<1xf32> | compiles | | !pto.partition_tensor_view<1x1xf32> | compiles | So the branch has never produced working code — it is reachable both from the DSL (`pl.system.cacheinvalid(t, [1, 1], off)`) and from InsertCommFence, whose MakeCacheInvalid uses the target's full shape and so hits it whenever a published tensor is itself 1x1. The special case existed only because the region path was broken on ptoas <= 0.51 (hw-native-sys/PTOAS#995). v0.52 fixes that lowering (PTOAS#1001), so a 1x1 partition_view is now correct: verified against the real 0.52 binary, a [1, 1] region at offsets [0, 8] over a [16, 16] f32 tensor emits GlobalTensor<float, Shape<1,1,1,1,1>, Stride<16,16,16,16,1>> at v1 + 8 and binds the new GlobalTensor overload via tensor.data(). Delete the branch: all region sizes take the partition_view path, leaving one construction and one emit site. GetFlatOffsetSSA and GetTensorBasePtr stay in use elsewhere, so nothing is orphaned. test_cacheinvalid_scalar_write_emits_ptr asserted the broken form ("partition_tensor_view" not in cmo_line), so it could not survive the fix as written. It is folded into a parametrized test_cacheinvalid_region_emits_partition_view covering both sizes, and the dynamic-offset test now asserts the partition-view operand instead of pto.addptr. Both DSL/IR docstrings and the en/zh operator tables dropped their description of the two-form split.
The pass doc stated that removing the wait-side `cacheinvalid all` from the ring-allreduce `.pto` "is rejected". That was measured on ptoas 0.50 and no longer holds. Re-run on 0.52 against the same `ring_step` kernel from tests/st/distributed/collectives/test_l3_allreduce_ring.py: unmodified accepted wait-side `cacheinvalid all` removed accepted every `cacheinvalid` and `system.fence` removed accepted All three compile with no diagnostic; the markers' instructions are simply absent from the generated C++ (region -> PTOAS__DCCI_SINGLE_CACHE_LINE, whole-GM -> dcci(..., ENTIRE_DATA_CACHE), fence -> pipe_barrier + dsb). Qualify the original claim with the version it was measured on and add a section stating there is no compile-time gate, so a missing marker is a data race rather than a build error. It also warns against reading a green test run as evidence a marker is unnecessary, citing the pass's own history: under the 0.50 pin the publish-side region cacheinvalid emitted no call at all (hw-native-sys/PTOAS#995) yet the distributed suite stayed green. The overview said ptoas "enforces" the contract, which reads as a check; "defines" plus an explicit note that ptoas does not verify the markers.
📝 WalkthroughWalkthroughThe change adds the ChangesCommunication pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant IRPass as InsertCommFence
participant Codegen as Distributed codegen
participant Cache as system.cacheinvalid
participant Fence as system.fence
participant Notify as pld.system.notify
IRPass->>Cache: insert region or whole-GM invalidation
IRPass->>Fence: insert publishing fence
Codegen->>Cache: emit peer-region invalidation for remote writes
Codegen->>Fence: emit GM fence ordering
Fence-->>Notify: ordered release before notify
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6b79faa479
ℹ️ 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".
| // Conservative: an unregistered op name is a call to a user function whose body | ||
| // is not analysed interprocedurally — assume it may publish. Registered builtin | ||
| // and distributed ops fall through to false (handled above or non-publishing). | ||
| return !OpRegistry::GetInstance().IsRegistered(call->op_->name_); |
There was a problem hiding this comment.
When user code does a scalar pl.write/tensor.write into a DistributedTensor and then notifies a peer, ConvertTensorToTileOps intentionally leaves that op as tensor.write for GM/distributed tensors (see src/ir/transforms/convert_tensor_to_tile_ops_pass.cpp:2214), and PTO codegen lowers it to pto.store_scalar. This new predicate falls through for the registered tensor.write op, so InsertCommFence inserts no system.cacheinvalid or GM fence before the release notify, violating the same data-before-signal contract this pass is meant to enforce and allowing stale scalar data to be observed remotely. Add a tensor.write case for arg 0 with DistributedTensorType, and return that target from PublishingWriteTarget.
Useful? React with 👍 / 👎.
Comment on lines
+253
to
+254
| if tensor is None: | ||
| return _ir_ops.cacheinvalid(span=span) |
There was a problem hiding this comment.
In the public DSL wrapper, calls such as pl.system.cacheinvalid(shapes=[1], offsets=[0]) or pl.system.cacheinvalid(None, [1], [0]) silently take this branch and emit the whole-GM form, dropping the supplied region arguments. The lower-level IR wrapper added in the same change correctly raises when tensor is None but shapes/offsets are present, so this wrapper can turn a missing tensor argument into a broad cache invalidation instead of surfacing the invalid call. Mirror the IR wrapper's shapes is not None or offsets is not None check before returning the no-arg op.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/backend/common/pto_ops_distributed.cpp (1)
115-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate rank-slot load logic; reuse
EmitLoadRankPair.
EmitCommRemoteOffsetInline(L131-139) re-derives the same rank-slot index (kRankIdOffset / kWindowSlotStride) and re-emits the samepto.load_scalar %ctx[%slot] : !pto.ptr<i64> -> i64line thatEmitLoadRankPair(L804-815) already encapsulates and is used bypld.system.rank/pld.system.nranks. Having two independent implementations of this ABI-pinned offset computation risks drift ifcomm_layout::kRankIdOffset/kWindowSlotStridesemantics change.♻️ Suggested refactor
Forward-declare (or hoist)
EmitLoadRankPairabove the anonymous namespace soEmitCommRemoteOffsetInlinecan call it directly instead of re-derivingk_rank_idx/c_r/rk_pair:+static std::string EmitLoadRankPair(codegen::PTOCodegen& cg, const std::string& ctx_ssa); + namespace { ... std::string EmitCommRemoteOffsetInline(const std::string& ctx_ssa, const std::string& peer_ssa, const DataType& dtype, codegen::PTOCodegen& codegen) { ... - const std::string c_r = codegen.GetOrEmitConstant(k_rank_idx, DataType::INDEX); const std::string c_w = codegen.GetOrEmitConstant(k_win_idx, DataType::INDEX); - // rankId = low 32 bits of the (rankId, rankNum) 8-byte slot. - const std::string rk_pair = codegen.NewTemp(); - codegen.Emit(rk_pair + " = pto.load_scalar " + ctx_ssa + "[" + c_r + "] : !pto.ptr<i64> -> i64"); + // rankId = low 32 bits of the (rankId, rankNum) 8-byte slot. + const std::string rk_pair = EmitLoadRankPair(codegen, ctx_ssa); const std::string rk_i32 = codegen.NewTemp();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/common/pto_ops_distributed.cpp` around lines 115 - 159, Update EmitCommRemoteOffsetInline to reuse EmitLoadRankPair for loading the rank slot, removing its duplicated kRankIdOffset/kWindowSlotStride, constant, and pto.load_scalar logic; declare or move EmitLoadRankPair so it is visible at the call site, while preserving the existing truncation and index-cast behavior.src/ir/transforms/insert_comm_fence_pass.cpp (1)
115-130: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a comment for the submit/tensor-level branches or narrow them.
In the default pipeline,
ConvertTensorToTileOpsruns beforeInsertCommFenceand producesIncoreTileOps;pld.tensor.get/putare user-facing tensor-level ops lowered during that pass.Submitcan appear as first-class InCore IR frompl.submitted/spmd_submit, so theSubmit => kWritebranch is reachable, but thepld.tensor.get/putbranches are only defensive against missed or custom pipeline ordering. Add a short invariant comment or adjust the predicates to avoid handling node types that this pass should never see.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ir/transforms/insert_comm_fence_pass.cpp` around lines 115 - 130, Clarify the intended IR invariant in StmtEffect around the Submit and tensor-level operation handling: document that Submit is valid first-class InCore IR, while pld.tensor.get/put should not normally reach InsertCommFence after ConvertTensorToTileOps. If those defensive branches are present elsewhere in the implementation, either add a concise invariant comment or narrow their predicates to the supported post-conversion operation types without changing Submit’s kWrite behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/pass-doc-ordering.md:
- Line 59: The pass summary for 44-insert_comm_fence.md inaccurately describes
marker placement. Update the description to distinguish remote-write peer-region
invalidation from whole-GM invalidation on waits, and state that markers are
appended immediately after the relevant write or wait, including when notify
occurs in another loop, rather than claiming they are always between the write
and notify.
In `@docs/en/dev/passes/44-insert_comm_fence.md`:
- Around line 37-39: Update the local publishing-write documentation to describe
only window-bound DistributedTensorType destinations, excluding plain Tensor
destinations. In docs/en/dev/passes/44-insert_comm_fence.md lines 37-39, qualify
the local get bullet and update its table entry; apply the same qualification
and table-entry update in docs/zh-cn/dev/passes/44-insert_comm_fence.md lines
32-35.
- Around line 16-18: The documented consume-side behavior for successful
pld.system.ttest is not implemented because StmtEffect() only recognizes
pld.system.wait. Update StmtEffect() and its tests so successful ttest triggers
the same whole-GM system.cacheinvalid() behavior, preserving the existing wait
handling; apply the documentation update consistently in
docs/en/dev/passes/44-insert_comm_fence.md lines 16-18 and
docs/zh-cn/dev/passes/44-insert_comm_fence.md lines 13-15, or remove the ttest
claim from both documents if implementation support is not added.
In `@include/pypto/ir/transforms/passes.h`:
- Around line 828-846: Update the Doxygen comment for the InsertCommFence()
factory to document the opaque publishing-write case: Submit or an
unregistered/unanalysed call must receive a conservative whole-GM
system.cacheinvalid followed by system.fence. Preserve the existing descriptions
of local writes, remote writes, and waits.
In `@src/ir/op/sync_ops/sync.cpp`:
- Around line 92-113: Update the system.cacheinvalid registration comments,
set_description, and shapes argument description to remove references to
shape-based dispatch, pto.addptr, and the scalar/ptr form. Document that every
tensor sub-region, including single-element regions, lowers through
partition_tensor_view, while preserving the separate whole-GM no-argument
behavior.
In `@tests/ut/ir/transforms/test_insert_comm_fence.py`:
- Around line 13-25: Update the module docstring describing the pass rules:
include the remote-write behavior demonstrated by
test_remote_store_gets_fence_only, stating that remote_store/put receive a
fence-only insertion rather than being left unchanged, and document the
opaque-write rule. Replace the claim of two purely-local rules and the
remote-store-only unchanged behavior while keeping the local publishing-write
and wait descriptions accurate.
---
Nitpick comments:
In `@src/backend/common/pto_ops_distributed.cpp`:
- Around line 115-159: Update EmitCommRemoteOffsetInline to reuse
EmitLoadRankPair for loading the rank slot, removing its duplicated
kRankIdOffset/kWindowSlotStride, constant, and pto.load_scalar logic; declare or
move EmitLoadRankPair so it is visible at the call site, while preserving the
existing truncation and index-cast behavior.
In `@src/ir/transforms/insert_comm_fence_pass.cpp`:
- Around line 115-130: Clarify the intended IR invariant in StmtEffect around
the Submit and tensor-level operation handling: document that Submit is valid
first-class InCore IR, while pld.tensor.get/put should not normally reach
InsertCommFence after ConvertTensorToTileOps. If those defensive branches are
present elsewhere in the implementation, either add a concise invariant comment
or narrow their predicates to the supported post-conversion operation types
without changing Submit’s kWrite behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29387061-3856-44fb-837e-02d7ae9d3371
📒 Files selected for processing (35)
.claude/rules/pass-doc-ordering.md.github/workflows/ci.ymlCMakeLists.txtdocs/en/dev/distributed_ops.mddocs/en/dev/ir/05-operators.mddocs/en/dev/passes/44-insert_comm_fence.mddocs/zh-cn/dev/distributed_ops.mddocs/zh-cn/dev/ir/05-operators.mddocs/zh-cn/dev/passes/44-insert_comm_fence.mdinclude/pypto/codegen/pto/pto_codegen.hinclude/pypto/ir/transforms/pass_properties.hinclude/pypto/ir/transforms/passes.hinclude/pypto/ir/transforms/utils/op_predicates.hpython/bindings/modules/passes.cpppython/pypto/backend/_ptoas_locate.pypython/pypto/backend/pto_backend.pypython/pypto/ir/op/system_ops.pypython/pypto/ir/pass_manager.pypython/pypto/jit/decorator.pypython/pypto/language/op/system_ops.pypython/pypto/pypto_core/passes.pyipython/pypto/runtime/debug/pto_rebuild.pysrc/backend/common/pto_ops_distributed.cppsrc/backend/common/pto_ops_memory.cppsrc/codegen/pto/pto_codegen.cppsrc/ir/op/sync_ops/sync.cppsrc/ir/transforms/insert_comm_fence_pass.cppsrc/ir/transforms/utils/op_predicates.cpptests/ut/backend/test_ptoas_locate.pytests/ut/codegen/distributed/test_distributed_pto_codegen.pytests/ut/codegen/test_pto_codegen_ops.pytests/ut/ir/transforms/test_insert_comm_fence.pytests/ut/ir/transforms/test_pass_manager.pytests/ut/language/parser/test_system_ops.pytoolchain/versions.env
💤 Files with no reviewable changes (2)
- src/codegen/pto/pto_codegen.cpp
- include/pypto/codegen/pto/pto_codegen.h
| | 42 | `42-materialize_runtime_scopes.md` | Runs after the final Simplify; inserts AUTO RuntimeScopeStmt so orchestration codegen emits PTO2_SCOPE 1:1 | | ||
| | 43 | `43-classify_iter_arg_carry.md` | Last pass (classifies each Orchestration ForStmt iter_arg — trivial alias vs materialised rebind carry — and sizes manual-scope TaskId array carries; runs after MaterializeRuntimeScopes) | | ||
| | 43 | `43-classify_iter_arg_carry.md` | Classifies each Orchestration ForStmt iter_arg (trivial alias vs materialised rebind carry) and sizes manual-scope TaskId array carries; runs after MaterializeRuntimeScopes | | ||
| | 44 | `44-insert_comm_fence.md` | Last pass (distributed: inserts a whole-tensor system.cacheinvalid + GM system.fence between each publishing write and the pld.system.notify that releases it; runs after all statement-reordering passes so the inserted ops stay adjacent to notify through codegen) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the pass’s actual marker placement.
This summary says every publishing write gets a whole-tensor system.cacheinvalid plus fence “between” the write and notify. In reality, remote writes get the peer-region invalidation in codegen, waits also receive whole-GM invalidation, and markers are appended immediately after the relevant write/wait even when notify is in another loop. Please align this description with the pass documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/pass-doc-ordering.md at line 59, The pass summary for
44-insert_comm_fence.md inaccurately describes marker placement. Update the
description to distinguish remote-write peer-region invalidation from whole-GM
invalidation on waits, and state that markers are appended immediately after the
relevant write or wait, including when notify occurs in another loop, rather
than claiming they are always between the write and notify.
Comment on lines
+16
to
+18
| - **Consume side.** A cacheable GM load after `pto.comm.twait` (or a successful | ||
| `pto.comm.ttest`) requires an explicit `pto.cmo.cacheinvalid all | ||
| #pto.address_space<gm>` first, so the reader sees the peer's fresh write. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^|/)44-insert_comm_fence\.md$|insert_comm_fence|ttest|comm_fence'
echo
echo "== markdown excerpts =="
for f in docs/en/dev/passes/44-insert_comm_fence.md docs/zh-cn/dev/passes/44-insert_comm_fence.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,80p'
fi
done
echo
echo "== source references =="
rg -n "StmtEffect|insert.*comm|twait|ttest|system\.wait|system\.ttest|cacheinvalid|address_space<gm>" .Repository: hw-native-sys/pypto
Length of output: 485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | grep -EIn '44-insert_comm_fence\.md$|insert_comm_fence|ttest|comm_fence' || true
echo
echo "== markdown excerpts =="
for f in docs/en/dev/passes/44-insert_comm_fence.md docs/zh-cn/dev/passes/44-insert_comm_fence.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,100p'
fi
done
echo
echo "== transformer source excerpts =="
if [ -f src/ir/transforms/insert_comm_fence_pass.cpp ]; then
echo "--- src/ir/transforms/insert_comm_fence_pass.cpp ---"
awk '{printf "%6d\t%s\n", NR, $0}' src/ir/transforms/insert_comm_fence_pass.cpp | sed -n '1,240p'
fi
echo
echo "== tests excerpts =="
if [ -f tests/ut/ir/transforms/test_insert_comm_fence.py ]; then
echo "--- tests/ut/ir/transforms/test_insert_comm_fence.py ---"
awk '{printf "%6d\t%s\n", NR, $0}' tests/ut/ir/transforms/test_insert_comm_fence.py | sed -n '1,260p'
fi
echo
echo "== targeted references =="
grep -RIn -E 'StmtEffect|insert.*comm|twait|ttest|system\.wait|system\.ttest|cacheinvalid|address_space<gm>' src tests docs/en/dev/passes/44-insert_comm_fence.md docs/zh-cn/dev/passes/44-insert_comm_fence.md || trueRepository: hw-native-sys/pypto
Length of output: 50377
Reconcile the ttest consume-side contract with pass behavior.
StmtEffect() only classifies pld.system.wait as a consume-side point, so successful pld.system.ttest is not followed by the whole-GM system.cacheinvalid() described in the docs. Update the implementation/test coverage to include successful ttest, or remove the ttest claim from both English and Chinese docs.
📍 Affects 2 files
docs/en/dev/passes/44-insert_comm_fence.md#L16-L18(this comment)docs/zh-cn/dev/passes/44-insert_comm_fence.md#L13-L15
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/en/dev/passes/44-insert_comm_fence.md` around lines 16 - 18, The
documented consume-side behavior for successful pld.system.ttest is not
implemented because StmtEffect() only recognizes pld.system.wait. Update
StmtEffect() and its tests so successful ttest triggers the same whole-GM
system.cacheinvalid() behavior, preserving the existing wait handling; apply the
documentation update consistently in docs/en/dev/passes/44-insert_comm_fence.md
lines 16-18 and docs/zh-cn/dev/passes/44-insert_comm_fence.md lines 13-15, or
remove the ttest claim from both documents if implementation support is not
added.
Comment on lines
+37
to
+39
| - **after each local publishing write** — a window-bound `tile.store`, or a `get` | ||
| into a local destination: a whole-tensor **region** `system.cacheinvalid` of the | ||
| written target, **immediately followed by a `system.fence`**; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align get documentation with IsPublishingWrite.
Only window-bound DistributedTensorType destinations are classified as publishing writes; plain Tensor destinations are not.
docs/en/dev/passes/44-insert_comm_fence.md#L37-L39: qualify the localgetbullet as window-bound and update its table entry.docs/zh-cn/dev/passes/44-insert_comm_fence.md#L32-L35: apply the same qualification and update its table entry.
📍 Affects 2 files
docs/en/dev/passes/44-insert_comm_fence.md#L37-L39(this comment)docs/zh-cn/dev/passes/44-insert_comm_fence.md#L32-L35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/en/dev/passes/44-insert_comm_fence.md` around lines 37 - 39, Update the
local publishing-write documentation to describe only window-bound
DistributedTensorType destinations, excluding plain Tensor destinations. In
docs/en/dev/passes/44-insert_comm_fence.md lines 37-39, qualify the local get
bullet and update its table entry; apply the same qualification and table-entry
update in docs/zh-cn/dev/passes/44-insert_comm_fence.md lines 32-35.
Comment on lines
+828
to
+846
| /** | ||
| * @brief Insert the ptoas data-before-signal markers around cross-rank publish | ||
| * and consume points (all via `system.cacheinvalid`). | ||
| * | ||
| * The `pld.system.notify` itself needs no marker: | ||
| * - after each **local** publishing write (window-bound `tile.store`, or `get` | ||
| * into a local destination): a region `system.cacheinvalid` of the written | ||
| * region immediately followed by a GM `system.fence`; | ||
| * - after each **remote** publishing write (`remote_store` / `put`): only a GM | ||
| * `system.fence`. Its data lands at a peer-offset address whose offset is not | ||
| * yet expressible in the IR, so the peer-region `pto.cmo.cacheinvalid` is | ||
| * emitted by the op's codegen as a workaround; the release fence is always an | ||
| * explicit `system.fence` op inserted here (codegen must not embed it); | ||
| * - after each **wait**: a no-arg (whole-GM) `system.cacheinvalid`. | ||
| * | ||
| * The pass carries no control-flow state and is idempotent. Runs last, after all | ||
| * statement-reordering passes, so the markers stay adjacent through codegen. | ||
| */ | ||
| Pass InsertCommFence(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc gap: missing the "opaque write" (Submit/unregistered call) marker case.
pass_properties.h's kInsertCommFenceProperties comment and the python binding docstring (insert_comm_fence in passes.cpp) both document a fourth case: an opaque publishing write (Submit or an unregistered/un-analysed call) gets a conservative whole-GM system.cacheinvalid + system.fence. This Doxygen comment on the public C++ factory only lists local write / remote write / wait, so a reader relying on this doc alone would miss that behavior.
📝 Proposed addition
* - after each **wait**: a no-arg (whole-GM) `system.cacheinvalid`.
+ * - after each **opaque publishing write** (`Submit` / an unregistered or
+ * un-analysed call, with no single addressable region): a conservative
+ * whole-GM `system.cacheinvalid` + GM `system.fence`.
*
* The pass carries no control-flow state and is idempotent. Runs last, after all📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * @brief Insert the ptoas data-before-signal markers around cross-rank publish | |
| * and consume points (all via `system.cacheinvalid`). | |
| * | |
| * The `pld.system.notify` itself needs no marker: | |
| * - after each **local** publishing write (window-bound `tile.store`, or `get` | |
| * into a local destination): a region `system.cacheinvalid` of the written | |
| * region immediately followed by a GM `system.fence`; | |
| * - after each **remote** publishing write (`remote_store` / `put`): only a GM | |
| * `system.fence`. Its data lands at a peer-offset address whose offset is not | |
| * yet expressible in the IR, so the peer-region `pto.cmo.cacheinvalid` is | |
| * emitted by the op's codegen as a workaround; the release fence is always an | |
| * explicit `system.fence` op inserted here (codegen must not embed it); | |
| * - after each **wait**: a no-arg (whole-GM) `system.cacheinvalid`. | |
| * | |
| * The pass carries no control-flow state and is idempotent. Runs last, after all | |
| * statement-reordering passes, so the markers stay adjacent through codegen. | |
| */ | |
| Pass InsertCommFence(); | |
| /** | |
| * `@brief` Insert the ptoas data-before-signal markers around cross-rank publish | |
| * and consume points (all via `system.cacheinvalid`). | |
| * | |
| * The `pld.system.notify` itself needs no marker: | |
| * - after each **local** publishing write (window-bound `tile.store`, or `get` | |
| * into a local destination): a region `system.cacheinvalid` of the written | |
| * region immediately followed by a GM `system.fence`; | |
| * - after each **remote** publishing write (`remote_store` / `put`): only a GM | |
| * `system.fence`. Its data lands at a peer-offset address whose offset is not | |
| * yet expressible in the IR, so the peer-region `pto.cmo.cacheinvalid` is | |
| * emitted by the op's codegen as a workaround; the release fence is always an | |
| * explicit `system.fence` op inserted here (codegen must not embed it); | |
| * - after each **wait**: a no-arg (whole-GM) `system.cacheinvalid`. | |
| * - after each **opaque publishing write** (`Submit` / an unregistered or | |
| * un-analysed call, with no single addressable region): a conservative | |
| * whole-GM `system.cacheinvalid` + GM `system.fence`. | |
| * | |
| * The pass carries no control-flow state and is idempotent. Runs last, after all | |
| * statement-reordering passes, so the markers stay adjacent through codegen. | |
| */ | |
| Pass InsertCommFence(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/pypto/ir/transforms/passes.h` around lines 828 - 846, Update the
Doxygen comment for the InsertCommFence() factory to document the opaque
publishing-write case: Submit or an unregistered/unanalysed call must receive a
conservative whole-GM system.cacheinvalid followed by system.fence. Preserve the
existing descriptions of local writes, remote writes, and waits.
Comment on lines
+92
to
113
| // Register system.cacheinvalid (Cache Maintenance Operation). | ||
| // Two forms selected by arg count: | ||
| // - No args: invalidate the whole GM address space | ||
| // (`pto.cmo.cacheinvalid all #pto.address_space<gm>`). Used as the coarse | ||
| // data-before-signal release marker before a bare barrier notify, and on the | ||
| // consume side after a wait before the next cacheable GM read. | ||
| // - (tensor, shapes, offsets): invalidate one tensor sub-region; codegen | ||
| // dispatches on shapes: all-1 -> scalar-write ptr form (pto.addptr); | ||
| // otherwise -> tile-store partition-view form (pto.partition_view). | ||
| // Variadic arity (0 or 3), like system.syncall below: the three arguments | ||
| // below describe ONLY the region form; omitting all of them selects the | ||
| // whole-GM form. The registry does not enforce argument count. | ||
| REGISTER_OP("system.cacheinvalid") | ||
| .set_description( | ||
| "Invalidate cache lines for a tensor sub-region (ptr form when the region is a single " | ||
| "element, partition-view form otherwise)") | ||
| "Invalidate cache lines: whole GM when called with no args, else a tensor sub-region " | ||
| "(ptr form when the region is a single element, partition-view form otherwise)") | ||
| .set_op_category("SyncOp") | ||
| .add_argument("tensor", "Target tensor whose sub-region is invalidated") | ||
| .add_argument("shapes", "Per-dimension region sizes (N-D tuple; all 1 selects the scalar/ptr form)") | ||
| .add_argument("offsets", "Per-dimension start offsets (N-D tuple matching tensor rank)") | ||
| .add_argument("tensor", "Region form: target tensor whose sub-region is invalidated") | ||
| .add_argument("shapes", | ||
| "Region form: per-dimension region sizes (N-D tuple; all 1 selects the scalar/ptr form)") | ||
| .add_argument("offsets", "Region form: per-dimension start offsets (N-D tuple matching tensor rank)") | ||
| .f_deduce_type(DeduceUnknownType); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc/comment: still describes the removed scalar-ptr dispatch.
This comment block, set_description, and the shapes argument description all say codegen dispatches on shapes (all-1 → pto.addptr ptr form, otherwise → pto.partition_view). Per pto_ops_memory.cpp (L841-855), the scalar-write branch was removed — every region, single-element included, now lowers through partition_tensor_view. docs/en/dev/ir/05-operators.md was updated accordingly, but this registration's own comments/description were not.
📝 Proposed fix
-// Register system.cacheinvalid (Cache Maintenance Operation).
-// Two forms selected by arg count:
-// - No args: invalidate the whole GM address space
-// (`pto.cmo.cacheinvalid all `#pto.address_space`<gm>`). Used as the coarse
-// data-before-signal release marker before a bare barrier notify, and on the
-// consume side after a wait before the next cacheable GM read.
-// - (tensor, shapes, offsets): invalidate one tensor sub-region; codegen
-// dispatches on shapes: all-1 -> scalar-write ptr form (pto.addptr);
-// otherwise -> tile-store partition-view form (pto.partition_view).
+// Register system.cacheinvalid (Cache Maintenance Operation).
+// Two forms selected by arg count:
+// - No args: invalidate the whole GM address space
+// (`pto.cmo.cacheinvalid all `#pto.address_space`<gm>`). Used as the coarse
+// data-before-signal release marker before a bare barrier notify, and on the
+// consume side after a wait before the next cacheable GM read.
+// - (tensor, shapes, offsets): invalidate one tensor sub-region via a
+// `pto.partition_view` + `pto.cmo.cacheinvalid ... single_cache_line`,
+// regardless of region size (including single-element regions).
// Variadic arity (0 or 3), like system.syncall below: the three arguments
// below describe ONLY the region form; omitting all of them selects the
// whole-GM form. The registry does not enforce argument count.
REGISTER_OP("system.cacheinvalid")
.set_description(
- "Invalidate cache lines: whole GM when called with no args, else a tensor sub-region "
- "(ptr form when the region is a single element, partition-view form otherwise)")
+ "Invalidate cache lines: whole GM when called with no args, else a tensor sub-region "
+ "(always lowered via a partition-view)")
.set_op_category("SyncOp")
.add_argument("tensor", "Region form: target tensor whose sub-region is invalidated")
.add_argument("shapes",
- "Region form: per-dimension region sizes (N-D tuple; all 1 selects the scalar/ptr form)")
+ "Region form: per-dimension region sizes (N-D tuple)")
.add_argument("offsets", "Region form: per-dimension start offsets (N-D tuple matching tensor rank)")
.f_deduce_type(DeduceUnknownType);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Register system.cacheinvalid (Cache Maintenance Operation). | |
| // Two forms selected by arg count: | |
| // - No args: invalidate the whole GM address space | |
| // (`pto.cmo.cacheinvalid all #pto.address_space<gm>`). Used as the coarse | |
| // data-before-signal release marker before a bare barrier notify, and on the | |
| // consume side after a wait before the next cacheable GM read. | |
| // - (tensor, shapes, offsets): invalidate one tensor sub-region; codegen | |
| // dispatches on shapes: all-1 -> scalar-write ptr form (pto.addptr); | |
| // otherwise -> tile-store partition-view form (pto.partition_view). | |
| // Variadic arity (0 or 3), like system.syncall below: the three arguments | |
| // below describe ONLY the region form; omitting all of them selects the | |
| // whole-GM form. The registry does not enforce argument count. | |
| REGISTER_OP("system.cacheinvalid") | |
| .set_description( | |
| "Invalidate cache lines for a tensor sub-region (ptr form when the region is a single " | |
| "element, partition-view form otherwise)") | |
| "Invalidate cache lines: whole GM when called with no args, else a tensor sub-region " | |
| "(ptr form when the region is a single element, partition-view form otherwise)") | |
| .set_op_category("SyncOp") | |
| .add_argument("tensor", "Target tensor whose sub-region is invalidated") | |
| .add_argument("shapes", "Per-dimension region sizes (N-D tuple; all 1 selects the scalar/ptr form)") | |
| .add_argument("offsets", "Per-dimension start offsets (N-D tuple matching tensor rank)") | |
| .add_argument("tensor", "Region form: target tensor whose sub-region is invalidated") | |
| .add_argument("shapes", | |
| "Region form: per-dimension region sizes (N-D tuple; all 1 selects the scalar/ptr form)") | |
| .add_argument("offsets", "Region form: per-dimension start offsets (N-D tuple matching tensor rank)") | |
| .f_deduce_type(DeduceUnknownType); | |
| // Register system.cacheinvalid (Cache Maintenance Operation). | |
| // Two forms selected by arg count: | |
| // - No args: invalidate the whole GM address space | |
| // (`pto.cmo.cacheinvalid all `#pto.address_space`<gm>`). Used as the coarse | |
| // data-before-signal release marker before a bare barrier notify, and on the | |
| // consume side after a wait before the next cacheable GM read. | |
| // - (tensor, shapes, offsets): invalidate one tensor sub-region via a | |
| // `pto.partition_view` + `pto.cmo.cacheinvalid ... single_cache_line`, | |
| // regardless of region size (including single-element regions). | |
| // Variadic arity (0 or 3), like system.syncall below: the three arguments | |
| // below describe ONLY the region form; omitting all of them selects the | |
| // whole-GM form. The registry does not enforce argument count. | |
| REGISTER_OP("system.cacheinvalid") | |
| .set_description( | |
| "Invalidate cache lines: whole GM when called with no args, else a tensor sub-region " | |
| "(always lowered via a partition-view)") | |
| .set_op_category("SyncOp") | |
| .add_argument("tensor", "Region form: target tensor whose sub-region is invalidated") | |
| .add_argument("shapes", | |
| "Region form: per-dimension region sizes (N-D tuple)") | |
| .add_argument("offsets", "Region form: per-dimension start offsets (N-D tuple matching tensor rank)") | |
| .f_deduce_type(DeduceUnknownType); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ir/op/sync_ops/sync.cpp` around lines 92 - 113, Update the
system.cacheinvalid registration comments, set_description, and shapes argument
description to remove references to shape-based dispatch, pto.addptr, and the
scalar/ptr form. Document that every tensor sub-region, including single-element
regions, lowers through partition_tensor_view, while preserving the separate
whole-GM no-argument behavior.
Comment on lines
+13
to
+25
| The pass enforces the ptoas data-before-signal contract with two purely-local | ||
| rules (verified on ptoas 0.50); the ``notify`` itself needs no marker: | ||
|
|
||
| * **After every local publishing write** — a window-bound ``pl.store`` (or ``get`` | ||
| into a local destination): a whole-tensor region | ||
| ``pl.system.cacheinvalid(target, shape, [0, ...])`` immediately followed by | ||
| ``pl.system.fence()``. | ||
| * **After every wait**: a whole-GM ``pl.system.cacheinvalid()`` (no args). | ||
|
|
||
| The **remote** writes ``remote_store`` / ``put`` land at a peer-offset address and | ||
| are left untouched by the pass — their codegen emits a correct peer-region | ||
| cacheinvalid + fence itself (see the codegen tests). So a program whose only | ||
| publishing write is a ``remote_store`` is returned unchanged by this pass. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Module docstring contradicts its own tests on remote-write behavior.
The docstring claims remote writes (remote_store/put) are "left untouched by the pass" and that a remote-store-only program is "returned unchanged," but test_remote_store_gets_fence_only (Line 82) shows the pass inserts pl.system.fence() after the remote write — matching the "remote write → fence only" rule documented in passes.pyi/pass_manager.py. The docstring also omits the opaque-write rule entirely, understating the rule count as "two purely-local rules."
📝 Suggested docstring fix
-The pass enforces the ptoas data-before-signal contract with two purely-local
-rules (verified on ptoas 0.50); the ``notify`` itself needs no marker:
+The pass enforces the ptoas data-before-signal contract (verified on ptoas
+0.50); the ``notify`` itself needs no marker:
* **After every local publishing write** — a window-bound ``pl.store`` (or ``get``
into a local destination): a whole-tensor region
``pl.system.cacheinvalid(target, shape, [0, ...])`` immediately followed by
``pl.system.fence()``.
+* **After every remote publishing write** (``remote_store`` / ``put``) — only a
+ GM ``pl.system.fence()``; the peer-region cacheinvalid is emitted by the
+ op's own codegen, since the peer offset is not yet IR-expressible.
+* **After every opaque publishing write** (a call to an unregistered/
+ un-analysed user function) — a conservative whole-GM
+ ``pl.system.cacheinvalid()`` + ``pl.system.fence()``.
* **After every wait**: a whole-GM ``pl.system.cacheinvalid()`` (no args).
-
-The **remote** writes ``remote_store`` / ``put`` land at a peer-offset address and
-are left untouched by the pass — their codegen emits a correct peer-region
-cacheinvalid + fence itself (see the codegen tests). So a program whose only
-publishing write is a ``remote_store`` is returned unchanged by this pass.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The pass enforces the ptoas data-before-signal contract with two purely-local | |
| rules (verified on ptoas 0.50); the ``notify`` itself needs no marker: | |
| * **After every local publishing write** — a window-bound ``pl.store`` (or ``get`` | |
| into a local destination): a whole-tensor region | |
| ``pl.system.cacheinvalid(target, shape, [0, ...])`` immediately followed by | |
| ``pl.system.fence()``. | |
| * **After every wait**: a whole-GM ``pl.system.cacheinvalid()`` (no args). | |
| The **remote** writes ``remote_store`` / ``put`` land at a peer-offset address and | |
| are left untouched by the pass — their codegen emits a correct peer-region | |
| cacheinvalid + fence itself (see the codegen tests). So a program whose only | |
| publishing write is a ``remote_store`` is returned unchanged by this pass. | |
| The pass enforces the ptoas data-before-signal contract (verified on ptoas | |
| 0.50); the ``notify`` itself needs no marker: | |
| * **After every local publishing write** — a window-bound ``pl.store`` (or ``get`` | |
| into a local destination): a whole-tensor region | |
| ``pl.system.cacheinvalid(target, shape, [0, ...])`` immediately followed by | |
| ``pl.system.fence()``. | |
| * **After every remote publishing write** (``remote_store`` / ``put``) — only a | |
| GM ``pl.system.fence()``; the peer-region cacheinvalid is emitted by the | |
| op's own codegen, since the peer offset is not yet IR-expressible. | |
| * **After every opaque publishing write** (a call to an unregistered/ | |
| un-analysed user function) — a conservative whole-GM | |
| ``pl.system.cacheinvalid()`` + ``pl.system.fence()``. | |
| * **After every wait**: a whole-GM ``pl.system.cacheinvalid()`` (no args). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ut/ir/transforms/test_insert_comm_fence.py` around lines 13 - 25,
Update the module docstring describing the pass rules: include the remote-write
behavior demonstrated by test_remote_store_gets_fence_only, stating that
remote_store/put receive a fence-only insertion rather than being left
unchanged, and document the opaque-write rule. Replace the claim of two
purely-local rules and the remote-store-only unchanged behavior while keeping
the local publishing-write and wait descriptions accurate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reland the
InsertCommFencepass on ptoas 0.52, and collapse thesystem.cacheinvalidcodegen down to a single path.1. Reland InsertCommFence (reverts #2138)
#2138 backed out #2076 because ptoas could not lower the publish-side region
cacheinvalid:pto.cmo.cacheinvalid <partition_tensor_view> single_cache_linePTOAS__DCCI_SINGLE_CACHE_LINE(<GlobalTensor>), whose body casts to__gm__ void*— a conversionGlobalTensordoes not have, so every kernel carrying the op failed to compilehw-native-sys/PTOAS#1001 fixes this with a
GlobalTensoroverload that takes the address viatensor.data(), shipped in v0.52. Verified against the real 0.52 binary — the emitted C++ now carries both overloads and binds the object one:This is a plain revert of #2138 except for two deliberate deviations:
toolchain/versions.envstays at v0.52; the revert's restore of the v0.50 pin is dropped.LegalizeTileCastat slot 14 and shifted everything below it, soclassify_iter_arg_carrynow owns 43 andInsertCommFence— still dead last in the pipeline — takes 44.2. Route every
cacheinvalidregion throughpartition_viewThe scalar-write branch of
system.cacheinvalidcodegen emitted a bare pointer operand. Measured against ptoas 0.52:!pto.ptr, no type annotation ← what we emittedexpected ':'!pto.ptr, with type annotationaddptr must feed make_tensor_view, ...!pto.tensor_view<1xf32>!pto.partition_tensor_view<1x1xf32>So that branch has never produced working code. It is reachable from the DSL (
pl.system.cacheinvalid(t, [1, 1], off)) and, with this reland, fromInsertCommFencetoo —MakeCacheInvaliduses the target's full shape, so any published tensor that is itself 1x1 lands there. That is thetest_l3_tensor_allreduce_intrinsic[2-1]case in #2136.The special case only existed because the region path was broken on ptoas <= 0.51. Now that v0.52 lowers it correctly, a 1x1
partition_viewis right: verified on the real binary, a[1, 1]region at offsets[0, 8]over a[16, 16]f32 tensor emitsGlobalTensor<float, Shape<1,1,1,1,1>, Stride<16,16,16,16,1>>atv1 + 8. End-to-end, pypto's own generated.ptofor that case now compiles under 0.52 where it previously hit the parse error.The branch is therefore deleted — one construction, one emit site.
GetFlatOffsetSSAandGetTensorBasePtrremain in use elsewhere, so nothing is orphaned.This supersedes #2137, which fixes the same bug by routing the scalar case through
tensor_view<1xT>and converging the two branches on a shared emit. Both work on 0.52; this one removes the branch entirely. Closing one of the two is a call for the authors — see "Open questions".test_cacheinvalid_scalar_write_emits_ptrasserted the broken form ("partition_tensor_view" not in cmo_line) and could not survive the fix as written. It is folded into a parametrizedtest_cacheinvalid_region_emits_partition_viewcovering both sizes; the dynamic-offset test now asserts the partition-view operand instead ofpto.addptr.3. Document that ptoas does not gate the markers
The pass doc claimed that removing the wait-side
cacheinvalid allfrom the ring-allreduce.pto"is rejected". That was measured on 0.50 and no longer holds. Re-run on 0.52 against the samering_stepkernel fromtests/st/distributed/collectives/test_l3_allreduce_ring.py:cacheinvalid allremovedcacheinvalidandsystem.fenceremovedAll three compile with no diagnostic; the instructions are simply absent from the generated C++. The doc now qualifies the original claim with the version it was measured on and adds a section stating there is no compile-time gate — a missing marker is a data race, not a build error — with an explicit warning against reading a green test run as evidence a marker is unnecessary.
Testing
ruff check/ruff format --check, en↔zh doc parity, English-only, header checksdist-system-testsnot run — needs a 2-device host. This is the meaningful acceptance gate: those 36 cases are what revert: InsertCommFence pass — buggy ptoas 0.50 dependency (#2076) #2138 backed the pass out for.Open questions
cacheinvalidemitted no call at all (PTOAS#995), so it never reached the device — and the distributed suite was green throughout. Combined with §3 (ptoas does not check for the markers), no existing test demonstrates what this pass fixes. Confirming it needs a case built to expose the race — large transfers, multiple ranks, repeated runs.Relates to #2076, #2136, #2138, hw-native-sys/PTOAS#995, hw-native-sys/PTOAS#1001