feat(language): add pl.graph, the in-place form of @pl.jit.graph - #2628
Conversation
|
Important Review skippedToo many files! This PR contains 215 files, which is 115 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (215)
You can disable this status message by setting the 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c812299a3d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
e0101ce to
01b6758
Compare
Marking a decoder layer as a recordable graph has so far required lifting it
into its own `@pl.jit.graph` function. That is the wrong shape when the region
is a slice of a larger orchestration body: the lift is mechanical, it invents a
name and a parameter list the author did not want, and it turns a one-line
annotation into an unreviewable diff.
`with pl.graph("name"):` marks the region where it is written. A new
`OutlineGraphScopes` pass lifts it back out into a `FunctionType::Graph`
function named after the region, running immediately before
`OutlineIncoreScopes` — so the `pl.at` scopes inside it are outlined on exactly
the same terms as those in a hand-written Graph function, and everything
downstream (LegalizeGraphBoundary, the Graph verifier, orchestration codegen)
sees one representation rather than two. Placing the pass there also keeps the
new `GraphScopeStmt` alive across three passes instead of forty, which is what
the RFC identifies as the real cost of a scope-shaped carrier.
Parameter order is the one thing that does not carry over: the outliner appends
in capture order while the decorator form uses the declared signature. The two
boundaries are permutations of each other, and nothing downstream reads a
boundary parameter by position.
The region name is required, unlike every other scope kind's optional
`name_hint`. It becomes the outlined function's name, hence the emitted symbol,
hence the runtime's graph key, so an auto-generated name would silently change a
recorded graph's identity whenever an unrelated region was added earlier in the
file. Two regions asking for the same name are disambiguated rather than merged;
sharing one name would give two topologies one Definition and make the second
call replay the first one's graph.
Four placements are compile errors rather than silent degradation, because the
runtime's answer to each is to fall back to ordinary submits with correct
numbers and no diagnostic — the feature would simply stop paying, invisibly. A
Graph region is rejected when nested in another Graph region, when nested in
`pl.at` / `pl.cluster` / `pl.spmd` (those become one device task, whereas a
Graph region records a topology of them), when written in a device kernel body,
and when given no name.
The outliner resolves its callees. A capture a region only ever hands to an
inner kernel's Out/InOut slot is otherwise left at the seeded In: write evidence
comes from the operator registry, which knows nothing about a GlobalVar callee.
Under-declaring is the silent direction — an In boundary tensor is not a writer,
so it loses its RAW edge, where over-declaring only over-orders. A Graph region
is where this bites, its body being a topology of kernel calls rather than of
registry-backed tile ops. ConvertTensorToTileOps repaired it later by accident;
a tensor-to-tile pass is not where param directions should be decided.
Resolving callees also makes the read collector's Submit walk matter. That
override replaces the base walk instead of extending it, and so dropped
`core_num_` and `predicate_` — both first-class SSA operands, not metadata. It
was harmless only while no outliner passed a program: with no callee to resolve,
no argument was ever skipped and every capture was read through its argument
anyway. Once an Out slot is skipped, a predicate can be a capture's only read,
and `pl.spmd_submit(k, rc, predicate=(rc[0,0] > 0))` — the predicate reading the
same value the launch overwrites — yielded Out where the region does read rc on
the way in. Both are now visited.
ScopeOutliner no longer recomputes a block's tail-use set at every position.
That set is read only by the target-scope paths, so it is now computed at the
positions that are, or contain, a scope of the kind being outlined — found by
one linear presence walk. A region among ordinary statements drops from
quadratic to linear (800 statements: 185ms to 10ms), which is what puts the pass
inside the O(N log N) bound of .claude/rules/pass-complexity.md for programs
that use the feature, rather than only for those that skip the outliner
entirely. The computed value is unchanged, and the win reaches the InCore,
hierarchy and cluster outliners too.
Inserting the pass at pipeline position 8 renumbers pass docs 08-49 to 09-50,
and the pass numbers named in prose across docs/ are re-synced with it.
Implements the P3 scope-form item of RFC hw-native-sys#2399.
01b6758 to
e2a70f1
Compare
What
with pl.graph("name"):marks a repeated orchestration region as a recordablegraph in place, instead of requiring it to be lifted into its own
@pl.jit.graphfunction first.Why
The decorator form is the wrong shape when the region is a slice of a larger
orchestration body. Lifting it is mechanical work that invents a name and a
parameter list the author did not want, and it turns a one-line annotation into
an unreviewable diff. The two surfaces are now an ergonomic choice, not a
semantic one: the decorator suits a layer that is already its own function, the
scope suits a region the author would rather leave where it is.
How
A new
OutlineGraphScopespass lifts eachGraphScopeStmtback out into aFunctionType::Graphfunction named after the region, leaving aCallbehind.It runs immediately before
OutlineIncoreScopes, which is the whole design:the
pl.atscopes inside a marked region are then outlined on exactly the sameterms as those inside a hand-written Graph function, and
LegalizeGraphBoundary,the Graph verifier and orchestration codegen see one representation rather than
two. Placing it there also keeps the new node alive across three passes instead
of forty — the RFC identifies that reach as the real cost of a scope-shaped
carrier.
Parameter order is the one thing that does not carry over: the outliner
appends in capture order while the decorator form uses the declared signature.
The two boundaries are permutations of each other, and nothing downstream reads
a boundary parameter by position.
The region name is required
Unlike every other scope kind's optional
name_hint,pl.graphdemands one. Itbecomes the outlined function's name, hence the emitted symbol, hence the
runtime's graph key — an auto-generated name would silently change a recorded
graph's identity whenever an unrelated region was added earlier in the file. Two
regions asking for the same name are disambiguated rather than merged; sharing
one name would give two topologies a single
GraphDefinitionand make the secondcall replay the first one's graph.
Rejected at compile time, not degraded silently
The runtime's answer to each of these is to fall back to ordinary submits: correct
numbers, no diagnostic, and the feature quietly stops paying. So each is a
compile error instead — a Graph region nested inside another Graph region, inside
pl.at/pl.cluster/pl.spmd(those become one device task, whereas a Graphregion records a topology of them), written in a device kernel body, or given no
name.
Pass renumbering
Inserting the pass at pipeline position 8 renumbers pass docs 08-49 → 09-50
(84 files, EN + ZH) per
.claude/rules/pass-doc-ordering.md, with everycross-reference updated. The renamed files' contents differ only in doc-number
references.
Testing
tests/ut/ir/statements/test_graph_scope_stmt.py— node construction,structural equality,
.ptoround-trip, print → reparse fixpointtests/ut/ir/transforms/test_outline_graph_scopes.py— outlining, structuralequivalence with the decorator form, name disambiguation, the loop case, the
OutlineIncoreScopeshand-off, and every rejection pathtests/st/runtime/framework_and_models/test_graph_execution.py— a scope-formdevice case sharing
test_per_layer_accumulate's golden (a frozen per-layeroffset gives 4.0 instead of 10.0), plus compile-side assertions that the region
really lowers to a Graph and presents the same boundary arity as the decorator
form
Full UT suite green.
clang-tidy(full-tree, sinceCMakeLists.txtchanged),clang-format,ruff,pyright,markdownlint, and the repository'scheck_ir_property_parity/check_docs_nav/check_docs_en_zh_parity/check_headers/check_english_only/check_no_broad_raiseslints all pass.The device cases in
test_graph_execution.pywere not run here — that file needshardware and
--forked; CI owns it.Scope
This is the
with pl.graph(...)half of RFC #2399's P3. The other half, scratch-arenapacking when a boundary exceeds 32 tensors, is not included and appears to be
obsolete: the runtime's cap is now
GRAPH_MAX_TENSOR_ARGS = 128/GRAPH_MAX_SCALAR_ARGS = 64, whichLegalizeGraphBoundaryalready encodes, so adecoder layer with twenty-odd weights sits far under it. Worth confirming before
that item is revived.
Related: #2399