refactor(ir): Remove InCoreScopeStmt and AutoInCoreScopeStmt#1062
refactor(ir): Remove InCoreScopeStmt and AutoInCoreScopeStmt#1062Hzfengsy wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
These two ScopeStmt subclasses overlapped with what HierarchyScopeStmt(level=CORE_GROUP) already expressed. After the typed class hierarchy refactor (hw-native-sys#1049) it became feasible to delete them as a contained sweep rather than a redesign. User surface - pl.incore() / pl.auto_incore() removed (deprecated since hw-native-sys#905) - pl.auto_chunk / chunked_loop_optimizer removed - pl.at(level=CORE_GROUP) is now the only inline form; carries split via optimizations=[pl.split(mode)] IR - ScopeKind trimmed to {Cluster, Hierarchy, Spmd} - HierarchyScopeStmt gains optional split_, validated at construction to only be set when level == CORE_GROUP - IRProperty SplitIncoreOrch and NoNestedInCore removed; downstream passes now require HierarchyOutlined, which means "no HierarchyScopeStmt remains in Opaque/Orchestration bodies" Outline passes - OutlineHierarchyScopes now handles non-CORE_GROUP scopes only; outlined function stays Opaque, parent type preserved - OutlineIncoreScopes (re-introduced under the same name) handles CORE_GROUP scopes only; outlined function is InCore and the parent is promoted Opaque -> Orchestration; HierarchyOutlined is produced here - Shared ScopeOutliner gains a HierarchyLevelFilter (Only|Exclude) so both passes share one mutator - Pipeline order: OutlineHierarchyScopes -> OutlineIncoreScopes -> OutlineClusterScopes - SplitChunkedLoops and InterchangeChunkLoops deleted (auto_chunk's former lowering) Cross-layer - C++ headers/impl, nanobind bindings (visitor/mutator trampoline counts updated 59->57 / 58->56), .pyi stubs, DSL parser, and tests all aligned - Pass docs renumbered (05 OutlineHierarchyScopes, 06 OutlineIncoreScopes, 07 OutlineClusterScopes, ...) in en/ and zh-cn/ 3506 unit tests pass; 16 skipped.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository 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:
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 |
![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 IR scope hierarchy by removing InCoreScopeStmt and AutoInCoreScopeStmt in favor of a unified HierarchyScopeStmt that supports an optional split_ field. Consequently, the SplitChunkedLoops and InterchangeChunkLoops passes have been removed, and the outlining logic has been split into OutlineHierarchyScopes and OutlineIncoreScopes. Feedback highlights several inconsistencies between the new implementation and its documentation, particularly regarding naming conventions and file paths. Additionally, an optimization was suggested to remove a redundant IR traversal in the OutlineIncoreScopes pass by utilizing existing outliner results.
| ``` | ||
|
|
||
| **Key points:** `chunk=C` splits the loop into an outer sequential loop and an inner loop of `C` iterations. The inner loop preserves the original kind (Sequential/Parallel/Unroll). `chunk` cannot be combined with `init_values`, and `chunk=` loops are only valid inside a `with pl.at(level=pl.Level.CORE_GROUP, optimizations=[pl.auto_chunk]):` — outside that scope the parser rejects them with an error. See [SplitChunkedLoops Pass](../passes/05-split_chunked_loops.md). | ||
| **Key points:** `chunk=C` splits the loop into an outer sequential loop and an inner loop of `C` iterations. The inner loop preserves the original kind (Sequential/Parallel/Unroll). `chunk` cannot be combined with `init_values`. |
There was a problem hiding this comment.
The documentation for chunk=C still claims that it splits the loop into outer and inner loops. However, the SplitChunkedLoops pass has been removed in this PR. This documentation is now stale and misleading. It should be updated to reflect that compiler-driven chunking via the chunk argument is no longer supported, or removed if the feature is entirely gone.
| ``` | ||
|
|
||
| **要点:** `chunk=C` 将循环拆分为外层顺序循环和 `C` 次迭代的内层循环。内层循环保留原始类型 (Sequential/Parallel/Unroll)。`chunk` 不能与 `init_values` 一起使用，且 `chunk=` 循环只能出现在 `with pl.at(level=pl.Level.CORE_GROUP, optimizations=[pl.auto_chunk]):` 内；在该作用域外，parser 会直接报错。参见 [SplitChunkedLoops Pass](../passes/05-split_chunked_loops.md)。 | ||
| **要点:** `chunk=C` 将循环拆分为外层顺序循环和 `C` 次迭代的内层循环。内层循环保留原始类型 (Sequential/Parallel/Unroll)。`chunk` 不能与 `init_values` 一起使用。 |
There was a problem hiding this comment.
此处关于 chunk=C 的说明仍然声称它会将循环拆分为外层和内层循环。然而，SplitChunkedLoops Pass 已在此 PR 中被移除。该文档现已过时且具有误导性。应更新以反映编译器不再支持通过 chunk 参数进行自动分块，或者如果该功能已完全移除，则应删除此说明。
| **Naming**: `{original_func}_{level}_{counter}` (e.g. `main_host_0`, | ||
| `main_global_0`). When `HierarchyScopeStmt.name_hint` is non-empty the hint | ||
| is used directly. |
There was a problem hiding this comment.
The naming scheme described here ({original_func}{level}{counter}) is inconsistent with the implementation in src/ir/transforms/outline_hierarchy_scopes_pass.cpp, which passes "hierarchy" as the suffix to ScopeOutliner. This results in names like main_hierarchy_0 instead of main_host_0. Please align the documentation with the code or update the code to use the level name in the suffix.
Also, the implementation filename mentioned on line 157 should be src/ir/transforms/outline_hierarchy_scopes_pass.cpp.
| **Naming**: `{original_func}_{level}_{counter}` (e.g. `main_host_0`, | |
| `main_global_0`). When `HierarchyScopeStmt.name_hint` is non-empty the hint | |
| is used directly. | |
| Naming: {original_func}_hierarchy_{counter} (e.g. main_hierarchy_0). When HierarchyScopeStmt.name_hint is non-empty the hint is used directly. |
References
- Consistent naming patterns for name_hint_ are essential for maintaining deterministic IR and stable test outputs, as variables are sorted by these hints.
| **命名规则**：`{原函数名}_{level}_{计数器}`（例如 `main_host_0`、 | ||
| `main_global_0`）。若 `HierarchyScopeStmt.name_hint` 非空，则直接使用该 | ||
| name_hint。 |
There was a problem hiding this comment.
此处描述的命名规则（{原函数名}{level}{计数器}）与 src/ir/transforms/outline_hierarchy_scopes_pass.cpp 中的实现不一致，代码中将 "hierarchy" 作为后缀传递给 ScopeOutliner。这会导致生成如 main_hierarchy_0 而非 main_host_0 的名称。请将文档与代码对齐，或更新代码以在后缀中使用 level 名称。
此外，第 151 行提到的实现文件名应为 src/ir/transforms/outline_hierarchy_scopes_pass.cpp。
| **命名规则**：`{原函数名}_{level}_{计数器}`（例如 `main_host_0`、 | |
| `main_global_0`）。若 `HierarchyScopeStmt.name_hint` 非空，则直接使用该 | |
| name_hint。 | |
| 命名规则：{原函数名}_hierarchy_{计数器}（例如 main_hierarchy_0）。若 HierarchyScopeStmt.name_hint 非空，则直接使用该 name_hint。 |
References
- Consistent naming patterns for name_hint_ are essential for maintaining deterministic IR and stable test outputs, as variables are sorted by these hints.
| **Naming**: `{original_func}_core_group_{counter}` (e.g. | ||
| `main_core_group_0`). Outlined InCore functions use a `_incore_`-style | ||
| name suffix in their attrs and are easily identifiable in printed IR. When | ||
| `HierarchyScopeStmt.name_hint` is non-empty the hint is used directly. |
There was a problem hiding this comment.
The naming scheme described here ({original_func}core_group{counter}) is inconsistent with the implementation in src/ir/transforms/outline_incore_scopes_pass.cpp, which uses "incore" as the suffix. This results in names like main_incore_0 instead of main_core_group_0.
Also, the implementation filename mentioned on line 185 should be src/ir/transforms/outline_incore_scopes_pass.cpp.
| **Naming**: `{original_func}_core_group_{counter}` (e.g. | |
| `main_core_group_0`). Outlined InCore functions use a `_incore_`-style | |
| name suffix in their attrs and are easily identifiable in printed IR. When | |
| `HierarchyScopeStmt.name_hint` is non-empty the hint is used directly. | |
| Naming: {original_func}_incore_{counter} (e.g. main_incore_0). Outlined InCore functions use a _incore_-style name suffix in their attrs and are easily identifiable in printed IR. When HierarchyScopeStmt.name_hint is non-empty the hint is used directly. |
References
- Consistent naming patterns for name_hint_ are essential for maintaining deterministic IR and stable test outputs, as variables are sorted by these hints.
| **命名规则**：`{原函数名}_core_group_{计数器}`（例如 | ||
| `main_core_group_0`）。提取出的 InCore 函数在 attrs 中使用 `_incore_` | ||
| 风格的名称后缀，在打印的 IR 中便于识别。若 | ||
| `HierarchyScopeStmt.name_hint` 非空，则直接使用该 name_hint。 |
There was a problem hiding this comment.
此处描述的命名规则（{原函数名}core_group{计数器}）与 src/ir/transforms/outline_incore_scopes_pass.cpp 中的实现不一致，代码中使用了 "incore" 作为后缀。这会导致生成如 main_incore_0 的名称。
此外，第 175 行提到的实现文件名应为 src/ir/transforms/outline_incore_scopes_pass.cpp。
| **命名规则**：`{原函数名}_core_group_{计数器}`（例如 | |
| `main_core_group_0`）。提取出的 InCore 函数在 attrs 中使用 `_incore_` | |
| 风格的名称后缀，在打印的 IR 中便于识别。若 | |
| `HierarchyScopeStmt.name_hint` 非空，则直接使用该 name_hint。 | |
| 命名规则：{原函数名}_incore_{计数器}（例如 main_incore_0）。提取出的 InCore 函数在 attrs 中使用 _incore_ 风格的名称后缀，在打印的 IR 中便于识别。若 HierarchyScopeStmt.name_hint 非空，则直接使用该 name_hint。 |
References
- Consistent naming patterns for name_hint_ are essential for maintaining deterministic IR and stable test outputs, as variables are sorted by these hints.
| | ------ | ---------------------- | ------------------- | -------------------- | | ||
| | Scope kind | `HierarchyScopeStmt` (non-CORE_GROUP) | `HierarchyScopeStmt` (CORE_GROUP) | `ClusterScopeStmt` / standalone `SpmdScopeStmt` | | ||
| | Output function type | `FunctionType::Opaque` | `FunctionType::InCore` | `FunctionType::Group` / `FunctionType::Spmd` | | ||
| | Naming pattern | `{func}_{level}_{n}` | `{func}_core_group_{n}` | `{func}_cluster_{n}` / `{func}_spmd_{n}` | |
There was a problem hiding this comment.
The naming patterns in this table are inconsistent with the implementation suffixes used in the corresponding passes.
| | Naming pattern | `{func}_{level}_{n}` | `{func}_core_group_{n}` | `{func}_cluster_{n}` / `{func}_spmd_{n}` | | |
| | Naming pattern | {func}_hierarchy_{n} | {func}_incore_{n} | {func}_cluster_{n} / {func}_spmd_{n} | |
References
- Consistent naming patterns for name_hint_ are essential for maintaining deterministic IR and stable test outputs, as variables are sorted by these hints.
| auto new_func = MutableCopy(func); | ||
| new_func->body_ = new_body; | ||
| new_func->func_type_ = new_func_type; | ||
| if (finder.found) { |
There was a problem hiding this comment.
The CoreGroupHierarchyFinder pass over the IR is redundant. The ScopeOutliner already performs a traversal and its result (outliner.GetOutlinedFunctions()) can be used to determine if any CORE_GROUP scopes were outlined. Removing this extra traversal improves efficiency and simplifies the code. Please also remove the CoreGroupHierarchyFinder class definition and its usage on lines 85-86.
| if (finder.found) { | |
| if (!outliner.GetOutlinedFunctions().empty()) { |
References
- Avoid redundant IR traversals and transformations to improve compiler efficiency and reduce unnecessary processing.
1 participant
Summary
Removes
InCoreScopeStmtandAutoInCoreScopeStmt(deprecated since #905). After the typed-class-hierarchy refactor in #1049 this is a contained deletion sweep — every CORE_GROUP region now flows throughHierarchyScopeStmt(level=CORE_GROUP), andOutlineIncoreScopes(re-introduced under the same name) is a small dedicated pass that turns those scopes intoFunction(InCore)and promotes the parentOpaque → Orchestration.~117 files, +1,784 / −8,796 lines.
What goes away
pl.incore(),pl.auto_incore(),pl.auto_chunk,chunked_loop_optimizerInCoreScopeStmt,AutoInCoreScopeStmt;ScopeKindtrimmed to{Cluster, Hierarchy, Spmd}SplitChunkedLoops,InterchangeChunkLoops(lowering forauto_chunk)IRProperty::SplitIncoreOrch,IRProperty::NoNestedInCore(folded intoHierarchyOutlined)What replaces / changes
with pl.incore(): ...with pl.at(level=pl.Level.CORE_GROUP): ...with pl.at(..., optimizations=[pl.split(m)])HierarchyScopeStmt(split=m)with pl.at(..., optimizations=[pl.auto_chunk])pl.range(..., chunk=…))HierarchyScopeStmtgained an optionalsplit_field, validated at construction to only be set whenlevel == CORE_GROUP.Outline-pass split
Per discussion mid-refactor, the InCore-emitting work is kept as its own pass to preserve its meaningful name:
OutlineHierarchyScopesHierarchyScopeStmtwherelevel != CORE_GROUPOpaqueOutlineIncoreScopesHierarchyScopeStmt(level=CORE_GROUP)InCoreOpaque → OrchestrationPipeline:
OutlineHierarchyScopes → OutlineIncoreScopes → OutlineClusterScopes. Both passes share oneScopeOutlinerconfigured with a newHierarchyLevelFilter{level, Mode::{Only,Exclude}}.Cross-layer
IRVisitor/IRMutatortrampoline counts adjusted 59→57 / 58→56),.pyistubs, DSL parser, all tests aligneddocs/en/dev/passes/anddocs/zh-cn/dev/passes/(05 OutlineHierarchyScopes, 06 OutlineIncoreScopes, 07 OutlineClusterScopes, …)AtContextTest plan
pl.at(level=CORE_GROUP, optimizations=[pl.split(...)])→ parse → print → reparse equals originalHierarchyOutlinedfires afterOutlineIncoreScopes; an Opaque-only program with a leftoverHierarchyScopeStmt(CORE_GROUP)(i.e. onlyOutlineHierarchyScopesran) fails the checkclang-tidy --diff-base HEADcleanNotes for reviewers
LoopOrigin::ChunkOuter/ChunkInner/ChunkRemainderenum values are retained but no built-in pass produces them anymore — left as user-attr-only (comment updated). Worth a follow-up issue if we want them removed entirely.HierarchyOutlinedis shared between the two outline passes; onlyOutlineIncoreScopesdeclares it asproduced(since CORE_GROUP scopes survive the first pass).