feat(passes): materialize distributed tensor comm ctx#1951
feat(passes): materialize distributed tensor comm ctx#1951hashiqiqixian wants to merge 6 commits into
Conversation
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a new IR transformation pass, MaterializeDistTensorCtx, which explicitly threads CommCtxType parameters/arguments through function signatures and call/submit sites for DistributedTensorType parameters. Updates PTO, distributed, and orchestration codegen backends to consume the explicit context, wires the pass into pipelines and Python bindings, and updates documentation and tests. ChangesMaterializeDistTensorCtx pass and codegen integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces the MaterializeDistTensorCtx pass, which materializes an explicit CommCtxType parameter and argument for each DistributedTensorType function parameter, making the communication context flow explicit in the IR. It updates the pipeline, codegen paths, and Python bindings accordingly, and adds comprehensive unit tests. However, a critical issue was identified in src/codegen/orchestration/orchestration_codegen.cpp, where widespread character encoding corruption occurred, introducing a Byte Order Mark (BOM) and corrupting various UTF-8 characters into garbage text.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b692c2a8
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ut/codegen/test_spmd_scope_tid_codegen.py (1)
39-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the stale
ext_signal_ctxexpectations in the two existingDistributedTensortests.
_codegen()now materializes dist-tensor ctx for every test in this class, so these cases should emitsignal_ctxdirectly, matching the new test. The assertions at lines 520 and 575 still look like pre-pass output and likely need to be changed toparams_t0.add_scalar(signal_ctx);.🤖 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/codegen/test_spmd_scope_tid_codegen.py` around lines 39 - 55, The `DistributedTensor` test expectations are stale because `_codegen()` now runs `materialize_dist_tensor_ctx()` before codegen, so the generated orchestration output should use the materialized `signal_ctx` form instead of the older `ext_signal_ctx`-style output. Update the two assertions in the existing `DistributedTensor` tests to match the new output from `codegen.generate_orchestration(...)`, specifically the `params_t0.add_scalar(signal_ctx);` form, so they align with the behavior exercised by `_codegen()`.
🤖 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 `@src/codegen/orchestration/orchestration_codegen.cpp`:
- Line 1: Remove the UTF-8 BOM from the start of orchestration_codegen.cpp so
the file begins directly with the header comment instead of the EF BB BF marker.
Keep the rest of the file unchanged; just save the file as plain UTF-8 without
BOM.
In `@src/ir/transforms/materialize_dist_tensor_ctx_pass.cpp`:
- Around line 74-90: BuildFunctionCtxPlan currently seeds used_names only from
func->params_, so MakeUniqueName can still generate a ctx parameter that
collides with a name already used in the function body. Update
BuildFunctionCtxPlan to also collect body-local names from the FunctionPtr
before creating ctx vars, and keep using MakeCtxParamBaseName plus
MakeUniqueName so ExtendFunctionSignature receives non-conflicting ctx parameter
names.
---
Outside diff comments:
In `@tests/ut/codegen/test_spmd_scope_tid_codegen.py`:
- Around line 39-55: The `DistributedTensor` test expectations are stale because
`_codegen()` now runs `materialize_dist_tensor_ctx()` before codegen, so the
generated orchestration output should use the materialized `signal_ctx` form
instead of the older `ext_signal_ctx`-style output. Update the two assertions in
the existing `DistributedTensor` tests to match the new output from
`codegen.generate_orchestration(...)`, specifically the
`params_t0.add_scalar(signal_ctx);` form, so they align with the behavior
exercised by `_codegen()`.
🪄 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
Run ID: 694d8adf-c528-486b-854e-59c132c4c6b8
📒 Files selected for processing (35)
.claude/rules/pass-doc-ordering.mdCMakeLists.txtdocs/en/dev/codegen/01-orchestration_codegen.mddocs/en/dev/language/00-python_syntax.mddocs/en/dev/passes/00-pass_manager.mddocs/en/dev/passes/37-materialize_comm_domain_scopes.mddocs/en/dev/passes/38-lower_host_tensor_collectives.mddocs/en/dev/passes/39-materialize_dist_tensor_ctx.mddocs/en/dev/passes/40-materialize_runtime_scopes.mddocs/en/user/01-language_guide.mddocs/zh-cn/dev/codegen/01-orchestration_codegen.mddocs/zh-cn/dev/language/00-python_syntax.mddocs/zh-cn/dev/passes/00-pass_manager.mddocs/zh-cn/dev/passes/37-materialize_comm_domain_scopes.mddocs/zh-cn/dev/passes/38-lower_host_tensor_collectives.mddocs/zh-cn/dev/passes/39-materialize_dist_tensor_ctx.mddocs/zh-cn/dev/passes/40-materialize_runtime_scopes.mddocs/zh-cn/user/01-language_guide.mdinclude/pypto/codegen/distributed/distributed_codegen.hinclude/pypto/codegen/pto/pto_codegen.hinclude/pypto/ir/transforms/pass_properties.hinclude/pypto/ir/transforms/passes.hpython/bindings/modules/passes.cpppython/pypto/backend/pto_backend.pypython/pypto/ir/pass_manager.pypython/pypto/pypto_core/passes.pyisrc/codegen/distributed/distributed_codegen.cppsrc/codegen/orchestration/orchestration_codegen.cppsrc/codegen/pto/pto_codegen.cppsrc/ir/transforms/materialize_dist_tensor_ctx_pass.cpptests/ut/codegen/distributed/test_distributed_pto_codegen.pytests/ut/codegen/distributed/test_host_orch_distributed.pytests/ut/codegen/test_spmd_scope_tid_codegen.pytests/ut/ir/transforms/test_materialize_dist_tensor_ctx.pytests/ut/ir/transforms/test_pass_manager.py
c40fffc to
68a878e
Compare
95f48f9 to
eec1b06
Compare
d3f07f8 to
ac7e15e
Compare
Summary
MaterializeDistTensorCtxto materialize explicitCommCtxTypeparams and args forDistributedTensorTypefunction params.LowerHostTensorCollectivesand before the finalSimplify.data_ctx.Testing
git diff --checkpython -m py_compile ...Note: full
pytestwas not run becausepytestis not installed in this environment.