revert: InsertCommFence pass — buggy ptoas 0.50 dependency (#2076) - #2138
Merged
zhangqi-chen merged 1 commit intoJul 25, 2026
Merged
Conversation
…-before-signal (hw-native-sys#2076)" This reverts commit 60a3df4.
📝 WalkthroughWalkthroughThe change centralizes distributed remote-offset computation into per-dtype helpers, restricts ChangesDistributed remote-offset codegen
Cache invalidation and pass cleanup
Toolchain pin
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DistributedOp
participant PTOCodegen
participant CommRemoteOffset_dtype
participant RemoteView
DistributedOp->>PTOCodegen: Register remote offset dtype
PTOCodegen->>CommRemoteOffset_dtype: Emit helper(ctx, peer)
DistributedOp->>CommRemoteOffset_dtype: func.call(ctx, peer)
DistributedOp->>RemoteView: addptr and make_tensor_view(offset)
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@include/pypto/codegen/pto/pto_codegen.h`:
- Around line 533-548: Correct the stale caller references in the documentation:
in include/pypto/codegen/pto/pto_codegen.h lines 533-548, update the
EmitCommRemoteView reference to pto_ops_distributed.cpp; in
src/codegen/pto/pto_codegen.cpp lines 436-441, update the
EmitCommRemoteOffsetHelpers() comment to reference
src/backend/common/pto_ops_distributed.cpp. No code behavior changes are needed.
In `@src/backend/common/pto_ops_memory.cpp`:
- Around line 821-822: Update the diagnostic string in the system.cacheinvalid
argument-count check to report the established order `(tensor, shapes,
offsets)`, matching the handler and both Python layers; leave the validation
logic unchanged.
🪄 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: d21a1b34-5446-492e-85e1-1ac8df9c45ac
📒 Files selected for processing (27)
.claude/rules/pass-doc-ordering.mdCMakeLists.txtdocs/en/dev/distributed_ops.mddocs/en/dev/passes/43-insert_comm_fence.mddocs/zh-cn/dev/distributed_ops.mddocs/zh-cn/dev/passes/43-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/ir/op/system_ops.pypython/pypto/ir/pass_manager.pypython/pypto/language/op/system_ops.pypython/pypto/pypto_core/passes.pyisrc/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/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 (15)
- docs/zh-cn/dev/passes/43-insert_comm_fence.md
- docs/en/dev/passes/43-insert_comm_fence.md
- src/ir/transforms/insert_comm_fence_pass.cpp
- CMakeLists.txt
- python/pypto/ir/pass_manager.py
- python/pypto/pypto_core/passes.pyi
- tests/ut/codegen/test_pto_codegen_ops.py
- tests/ut/language/parser/test_system_ops.py
- tests/ut/ir/transforms/test_pass_manager.py
- include/pypto/ir/transforms/utils/op_predicates.h
- src/ir/transforms/utils/op_predicates.cpp
- python/bindings/modules/passes.cpp
- include/pypto/ir/transforms/pass_properties.h
- include/pypto/ir/transforms/passes.h
- tests/ut/ir/transforms/test_insert_comm_fence.py
Comment on lines
+533
to
+548
| /** | ||
| * @brief Register a dtype that needs a ``@CommRemoteOffset_<dtype>`` | ||
| * helper, and return the helper function name. | ||
| * | ||
| * Called by op lowering code (``EmitCommRemoteView`` in | ||
| * ``pto_ops_common.cpp``) at the moment a ``func.call`` to the helper | ||
| * is emitted. Any op that routes peer addressing through | ||
| * ``EmitCommRemoteView`` automatically gets the matching helper emitted | ||
| * at module-flush time — no separate pre-walk of the IR is needed. | ||
| * | ||
| * Validates that the dtype is byte-sized (sub-byte dtypes have no | ||
| * whole-byte element stride and so have no well-defined cross-rank | ||
| * offset). Failing here surfaces the error at the op call site rather | ||
| * than at module-emission time. | ||
| */ | ||
| std::string RegisterCommRemoteOffsetHelper(const DataType& dtype); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix stale pto_ops_common.cpp reference — EmitCommRemoteView is defined in pto_ops_distributed.cpp. Both comments describe the same caller relationship but point to the wrong source file.
include/pypto/codegen/pto/pto_codegen.h#L533-L548: update theRegisterCommRemoteOffsetHelperdoc to sayEmitCommRemoteViewlives inpto_ops_distributed.cpp, notpto_ops_common.cpp.src/codegen/pto/pto_codegen.cpp#L436-L441: update the comment aboveEmitCommRemoteOffsetHelpers()to referencesrc/backend/common/pto_ops_distributed.cpp.
📍 Affects 2 files
include/pypto/codegen/pto/pto_codegen.h#L533-L548(this comment)src/codegen/pto/pto_codegen.cpp#L436-L441
🤖 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/codegen/pto/pto_codegen.h` around lines 533 - 548, Correct the
stale caller references in the documentation: in
include/pypto/codegen/pto/pto_codegen.h lines 533-548, update the
EmitCommRemoteView reference to pto_ops_distributed.cpp; in
src/codegen/pto/pto_codegen.cpp lines 436-441, update the
EmitCommRemoteOffsetHelpers() comment to reference
src/backend/common/pto_ops_distributed.cpp. No code behavior changes are needed.
Comment on lines
821
to
+822
| INTERNAL_CHECK_SPAN(op->args_.size() == 3, op->span_) | ||
| << "system.cacheinvalid takes 0 (whole-GM) or 3 arguments (tensor, shapes, offsets), got " | ||
| << op->args_.size(); | ||
| << "system.cacheinvalid takes 3 arguments (tensor, offsets, shapes), got " << op->args_.size(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the argument order in the diagnostic.
Line 822 says (tensor, offsets, shapes), but this handler—and both Python layers—use (tensor, shapes, offsets). Invalid IR will be diagnosed with the wrong call order.
Proposed fix
- << "system.cacheinvalid takes 3 arguments (tensor, offsets, shapes), got " << op->args_.size();
+ << "system.cacheinvalid takes 3 arguments (tensor, shapes, offsets), got " << op->args_.size();📝 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
| INTERNAL_CHECK_SPAN(op->args_.size() == 3, op->span_) | |
| << "system.cacheinvalid takes 0 (whole-GM) or 3 arguments (tensor, shapes, offsets), got " | |
| << op->args_.size(); | |
| << "system.cacheinvalid takes 3 arguments (tensor, offsets, shapes), got " << op->args_.size(); | |
| INTERNAL_CHECK_SPAN(op->args_.size() == 3, op->span_) | |
| << "system.cacheinvalid takes 3 arguments (tensor, shapes, offsets), got " << op->args_.size(); |
🤖 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_memory.cpp` around lines 821 - 822, Update the
diagnostic string in the system.cacheinvalid argument-count check to report the
established order `(tensor, shapes, offsets)`, matching the handler and both
Python layers; leave the validation logic unchanged.
zhangqi-chen
approved these changes
Jul 25, 2026
This was referenced Jul 27, 2026
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
2 participants
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.
Summary
Reverts #2076 (
feat(passes): InsertCommFence — cacheinvalid + fence for data-before-signal, commit60a3df40). That change targets ptoas 0.50, whose current build has a bug, so the pass, its codegen changes, and the toolchain bump are being backed out until a fixed ptoas is available.This restores the pre-#2076 state:
InsertCommFencepass (src/ir/transforms/insert_comm_fence_pass.cpp), its Default/DebugTile pipeline registration, bindings, stubs, and docs (43-insert_comm_fence.md).CommRemoteOffsetinlining and the distributed remote-op codegen changes.toolchain/versions.envand the TPUT drain barrier.Doc-ordering fix (not a plain revert)
The original #2076 diff to
.claude/rules/pass-doc-ordering.mdbundled two table rows: entry 42 (classify_iter_arg_carry, from an earlier commit whose row was missing) and entry 43 (insert_comm_fence). A mechanical revert would drop both. Since theClassifyIterArgCarrypass still exists and runs last in the pipeline, entry 42 is kept (relabelled "Last pass") and only entry 43 is removed.Testing
insert_comm_fencefully removed frompass_manager.py;classify_iter_arg_carryis now the last pass