-
Notifications
You must be signed in to change notification settings - Fork 34
[Perf] Streams 3: Add qd.stream_parallel() context manager #409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 33 commits
a40ed4c
aa2fa2a
be7ad92
ce83281
880abc7
065a3b7
cfc6f39
e9ce144
007b050
91ca883
8cd793c
e880d07
ad720bb
6351215
470912f
3b0ba29
1c62eae
e55c84f
216f7d5
49dc5af
d7836e3
74604f2
b83b65d
0c552cd
212aeb9
226c7c5
1f471b3
6919fee
7b4e2a4
88f1bf7
ca560b6
158c8fb
388a797
df0b03a
acff351
70eb471
ebd5e11
a6c3852
04e18ba
3af5bc8
55b71fb
dbb055c
af4a306
c50d034
824cabf
24bc67d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| from quadrants.lang.ast.ast_transformers.function_def_transformer import ( | ||
| FunctionDefTransformer, | ||
| ) | ||
| from quadrants.lang.ast.symbol_resolver import ASTResolver | ||
| from quadrants.lang.exception import ( | ||
| QuadrantsIndexError, | ||
| QuadrantsRuntimeTypeError, | ||
|
|
@@ -39,6 +40,7 @@ | |
| from quadrants.lang.field import Field | ||
| from quadrants.lang.matrix import Matrix, MatrixType | ||
| from quadrants.lang.snode import append, deactivate, length | ||
| from quadrants.lang.stream import stream_parallel | ||
| from quadrants.lang.struct import Struct, StructType | ||
| from quadrants.lang.util import ( | ||
| is_from_quadrants_module as _is_from_quadrants_module, | ||
|
|
@@ -119,7 +121,11 @@ def build_AnnAssign(ctx: ASTTransformerFuncContext, node: ast.AnnAssign): | |
|
|
||
| @staticmethod | ||
| def build_assign_annotated( | ||
| ctx: ASTTransformerFuncContext, target: ast.Name, value, is_static_assign: bool, annotation: Type | ||
| ctx: ASTTransformerFuncContext, | ||
| target: ast.Name, | ||
| value, | ||
| is_static_assign: bool, | ||
| annotation: Type, | ||
| ): | ||
| """Build an annotated assignment like this: target: annotation = value. | ||
|
|
||
|
|
@@ -165,7 +171,10 @@ def build_Assign(ctx: ASTTransformerFuncContext, node: ast.Assign) -> None: | |
|
|
||
| @staticmethod | ||
| def build_assign_unpack( | ||
| ctx: ASTTransformerFuncContext, node_target: list | ast.Tuple, values, is_static_assign: bool | ||
| ctx: ASTTransformerFuncContext, | ||
| node_target: list | ast.Tuple, | ||
| values, | ||
| is_static_assign: bool, | ||
| ): | ||
| """Build the unpack assignments like this: (target1, target2) = (value1, value2). | ||
| The function should be called only if the node target is a tuple. | ||
|
|
@@ -591,7 +600,8 @@ def build_Return(ctx: ASTTransformerFuncContext, node: ast.Return) -> None: | |
| else: | ||
| raise QuadrantsSyntaxError("The return type is not supported now!") | ||
| ctx.ast_builder.create_kernel_exprgroup_return( | ||
| expr.make_expr_group(return_exprs), _qd_core.DebugInfo(ctx.get_pos_info(node)) | ||
| expr.make_expr_group(return_exprs), | ||
| _qd_core.DebugInfo(ctx.get_pos_info(node)), | ||
| ) | ||
| else: | ||
| ctx.return_data = node.value.ptr | ||
|
|
@@ -1520,6 +1530,24 @@ def build_Continue(ctx: ASTTransformerFuncContext, node: ast.Continue) -> None: | |
| ctx.ast_builder.insert_continue_stmt(_qd_core.DebugInfo(ctx.get_pos_info(node))) | ||
| return None | ||
|
|
||
| @staticmethod | ||
| def build_With(ctx: ASTTransformerFuncContext, node: ast.With) -> None: | ||
| if len(node.items) != 1: | ||
| raise QuadrantsSyntaxError("'with' in Quadrants kernels only supports a single context manager") | ||
| item = node.items[0] | ||
| if item.optional_vars is not None: | ||
| raise QuadrantsSyntaxError("'with ... as ...' is not supported in Quadrants kernels") | ||
| if not isinstance(item.context_expr, ast.Call): | ||
| raise QuadrantsSyntaxError("'with' in Quadrants kernels requires a call expression") | ||
| if not ASTResolver.resolve_to(item.context_expr.func, stream_parallel, ctx.global_vars): | ||
| raise QuadrantsSyntaxError("'with' in Quadrants kernels only supports qd.stream_parallel()") | ||
|
Comment on lines
+1538
to
+1541
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 qd.stream_parallel() context manager accepts but silently drops user-supplied args/kwargs. Extended reasoning...What the bug is
Meanwhile @contextmanager
def stream_parallel():
yieldSo the only legal call shape is Step-by-step proof@qd.kernel
def k():
with qd.stream_parallel(123, num_streams=4): # silently accepted
for i in range(N):
a[i] = 1.0
with qd.stream_parallel(): # also accepted
for j in range(N):
b[j] = 2.0
Why nothing else catches itThe contextmanager body never executes at Python runtime (the AST transformer rewrites the call before ImpactUser-facing UX papercut. A user who reasonably guesses at an API like FixOne-line addition in if item.context_expr.args or item.context_expr.keywords:
raise QuadrantsSyntaxError("qd.stream_parallel() takes no arguments")Three independent verifiers reviewed this and confirmed it is correct, real, and a real (if minor) UX gap; all three suggested nit severity given there is no correctness or performance impact. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still applies — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still applies — |
||
| if not ctx.is_kernel: | ||
| raise QuadrantsSyntaxError("qd.stream_parallel() can only be used inside @qd.kernel, not @qd.func") | ||
| ctx.ast_builder.begin_stream_parallel() | ||
| build_stmts(ctx, node.body) | ||
|
Comment on lines
+1531
to
+1545
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Extended reasoning...What the bug is
Result: a Step-by-step proof@qd.kernel
def k(cond: qd.i32):
if cond:
with qd.stream_parallel():
for i in range(N):
a[i] = 1.0
for j in range(N):
b[j] = 2.0
The two for-loops execute serially on the default stream — exactly the behavior Same trigger on other containersIdentical silent-serialization manifests for any non-root container:
Why nothing else catches it
The validator's stated contract ('all top-level statements must be Severity rationaleMarking normal. The validator was specifically introduced in this PR to give clear compile-time errors for misuse patterns that would otherwise be silently confusing. A new user who reasonably writes FixTwo reasonable options, both one helper function: (a) Have (b) Strengthen Either brings the runtime behavior into line with the validator's documented contract. |
||
| ctx.ast_builder.end_stream_parallel() | ||
|
Comment on lines
+1544
to
+1546
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Non-for statements (e.g. Extended reasoning...Bug\n\n
|
||
| return None | ||
|
|
||
| @staticmethod | ||
| def build_Pass(ctx: ASTTransformerFuncContext, node: ast.Pass) -> None: | ||
| return None | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,11 +26,13 @@ | |
| from quadrants.lang.ast.ast_transformer_utils import ( | ||
| ASTTransformerFuncContext, | ||
| ) | ||
| from quadrants.lang.ast.symbol_resolver import ASTResolver | ||
| from quadrants.lang.buffer_view import BufferView | ||
| from quadrants.lang.exception import ( | ||
| QuadrantsSyntaxError, | ||
| ) | ||
| from quadrants.lang.matrix import MatrixType | ||
| from quadrants.lang.stream import stream_parallel | ||
| from quadrants.lang.struct import StructType | ||
| from quadrants.lang.util import to_quadrants_type | ||
| from quadrants.types import annotations, buffer_view_type, ndarray_type, primitive_types | ||
|
|
@@ -443,7 +445,39 @@ | |
| else: | ||
| FunctionDefTransformer._transform_as_func(ctx, node, args) | ||
|
|
||
| if ctx.is_kernel: | ||
| FunctionDefTransformer._validate_stream_parallel_exclusivity(node.body, ctx.global_vars) | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| with ctx.variable_scope_guard(): | ||
| build_stmts(ctx, node.body) | ||
|
|
||
| return None | ||
|
|
||
| @staticmethod | ||
| def _is_stream_parallel_with(stmt: ast.stmt, global_vars: dict[str, Any]) -> bool: | ||
| if not isinstance(stmt, ast.With): | ||
| return False | ||
| if len(stmt.items) != 1: | ||
| return False | ||
| item = stmt.items[0] | ||
|
Comment on lines
+511
to
+513
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is |
||
| if not isinstance(item.context_expr, ast.Call): | ||
| return False | ||
| return ASTResolver.resolve_to(item.context_expr.func, stream_parallel, global_vars) | ||
|
|
||
| @staticmethod | ||
| def _is_docstring(stmt: ast.stmt, index: int) -> bool: | ||
| return index == 0 and isinstance(stmt, ast.Expr) and isinstance(stmt.value, (ast.Constant, ast.Str)) | ||
|
|
||
| @staticmethod | ||
| def _validate_stream_parallel_exclusivity(body: list[ast.stmt], global_vars: dict[str, Any]) -> None: | ||
| if not any(FunctionDefTransformer._is_stream_parallel_with(s, global_vars) for s in body): | ||
| return | ||
| for i, stmt in enumerate(body): | ||
| if FunctionDefTransformer._is_docstring(stmt, i): | ||
| continue | ||
| if not FunctionDefTransformer._is_stream_parallel_with(stmt, global_vars): | ||
| raise QuadrantsSyntaxError( | ||
| "When using qd.stream_parallel(), all top-level statements " | ||
| "in the kernel must be 'with qd.stream_parallel():' blocks. " | ||
| "Move non-parallel code to a separate kernel." | ||
| ) | ||
|
Check warning on line 483 in python/quadrants/lang/ast/ast_transformers/function_def_transformer.py
|
||
|
Comment on lines
+572
to
+576
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't understand why you are moving to the next line before you have to. This is weird to me. But I don't care much.
claude[bot] marked this conversation as resolved.
Comment on lines
+530
to
+576
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new _validate_stream_parallel_exclusivity check (function_def_transformer.py:467-477) only carves out docstrings (index==0 ast.Expr(Constant)), but ast.Pass and Extended reasoning...What the bug is
Note: the original synthesis also lists Python How the failure manifests@qd.kernel
def k():
qd.static_assert(N > 0) # ast.Expr(Call(static_assert)) — no IR
with qd.stream_parallel():
for i in range(N): a[i] = 1.0
with qd.stream_parallel():
for j in range(N): b[j] = 2.0Step-by-step trace:
The user sees an error telling them to "move non-parallel code", but there is no non-parallel runtime code to move — Why no existing safeguard catches it
Impact and severityUser-facing impact is a confusing compile-time error. No silent miscompilation, no perf regression, no data race. Workaround is trivial (delete or relocate the directive). All four verifiers independently confirmed at nit severity:
FixExtend the existing @staticmethod
def _is_skippable(stmt: ast.stmt, index: int, global_vars: dict[str, Any]) -> bool:
if FunctionDefTransformer._is_docstring(stmt, index):
return True
if isinstance(stmt, ast.Pass):
return True
if isinstance(stmt, ast.Expr) and isinstance(stmt.value, ast.Call):
for target in (static_assert, static_print):
if ASTResolver.resolve_to(stmt.value.func, target, global_vars):
return True
return FalseThen call |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import weakref | ||
| from contextlib import contextmanager | ||
|
|
||
| from quadrants.lang import impl | ||
|
|
||
|
|
@@ -127,4 +128,14 @@ def create_event() -> Event: | |
| return Event(handle, _get_prog_weakref()) | ||
|
|
||
|
|
||
| __all__ = ["Stream", "Event", "create_stream", "create_event"] | ||
| @contextmanager | ||
| def stream_parallel(): | ||
| """Run top-level for loops in this block on separate GPU streams. | ||
|
|
||
| Used inside @qd.kernel. At Python runtime (outside kernels), this is a no-op. During kernel compilation, the AST | ||
| transformer calls into the C++ ASTBuilder to tag loops with a stream-parallel group ID. | ||
| """ | ||
|
Comment on lines
+178
to
+184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The docstring on Extended reasoning...What the docstring says vs. what the code does
@contextmanager
def stream_parallel():
"""Run top-level for loops in this block on separate GPU streams.
Used inside @qd.kernel. ...
"""
yieldThe plural 'separate GPU streams' for the for-loops within a single block reads naturally as 'each for-loop here gets its own stream'. That contradicts the actual semantics established by this same PR. Why it is wrong
void begin_stream_parallel() {
QD_ERROR_IF(current_stream_parallel_group_id_ != 0, ...);
current_stream_parallel_group_id_ = ++stream_parallel_group_counter_;
}Every for-loop inside the with-body then reads that single value. The launcher at Why I'm certain — corroborated by the PR's own docsThe user-guide rewrite in this same PR (
So the docstring directly contradicts the prose docs the same PR ships. Step-by-step proof — two for-loops in one block@qd.kernel
def k():
with qd.stream_parallel():
for i in range(N): a[i] = 1.0 # for-A
for j in range(N): b[j] = 2.0 # for-B
So the user-visible behavior for the docstring's described scenario ('top-level for loops in this block') is one stream, not multiple. The plural is wrong. FixReplace the docstring with text that matches the implementation and the .md docs, e.g.: @contextmanager
def stream_parallel():
"""Run this block on its own GPU stream, separate from sibling stream_parallel blocks.
Used inside @qd.kernel. Consecutive `with qd.stream_parallel():` blocks execute concurrently on
different streams; multiple for-loops inside one block share that block's stream and execute
serially on it. At Python runtime (outside kernels), this is a no-op — the AST transformer
interprets the with-statement at compile time and tags loops with a stream-parallel group ID.
"""
yieldSeverityAll four verifiers rated this nit: pure documentation papercut, no behavioral or correctness impact, the help() output for stream_parallel is the only user-facing surface affected. Worth folding in because the contradiction is right next to the PR's own .md prose. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment is unchanged at python/quadrants/lang/stream.py:133-137 — still reads "Run top-level for loops in this block on separate GPU streams" (plural). The contradiction with user_guide/streams.md ("Multiple for loops within a single block share a stream and run serially on it") still stands. |
||
| yield | ||
|
|
||
|
|
||
| __all__ = ["Stream", "Event", "create_stream", "create_event", "stream_parallel"] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -638,6 +638,7 @@ class TaskCodeGenCUDA : public TaskCodeGenLLVM { | |
| } | ||
| current_task->block_dim = stmt->block_dim; | ||
| current_task->dynamic_shared_array_bytes = dynamic_shared_array_bytes; | ||
| current_task->stream_parallel_group_id = stmt->stream_parallel_group_id; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 qd.deactivate() inside with qd.stream_parallel(): silently runs serially. Two complementary sites strip stream_parallel_group_id from the gc tasks the offloader inserts: (1) offload.cpp::insert_gc constructs the gc OffloadedStmt without propagating the preceding for-loop's group id (defaults to 0), and (2) emit_cuda_gc / emit_amdgpu_gc emit 3 sub-tasks via init_offloaded_task_function and never copy stmt->stream_parallel_group_id onto current_task — bypassing the codegen_cuda.cpp:641 / codegen_amdgpu.cpp:354 propagation that exists in the else branch. Both fixes are required: the IR-level fix alone has no observable effect because codegen still strips the id; the codegen fix alone never sees a non-zero id to propagate. Extended reasoning...What the bug isThis PR threads Site 1 — IR-level: auto gc_task = Stmt::make_typed<OffloadedStmt>(OffloadedStmt::TaskType::gc, config.arch, kernel);
gc_task->snode = snode; // stream_parallel_group_id default-initialized to 0
b->insert(std::move(gc_task), i + 1);
Site 2 — Codegen: Why both fixes are requiredA developer who applies only the site-1 fix sets a non-zero group_id on the gc OffloadedStmt — but codegen (site 2) immediately discards it when constructing the 3 OffloadedTasks. The launcher reads the OffloadedTask vector, so the user-observable batch-splitting symptom is unchanged. Conversely, the site-2 fix alone would faithfully propagate Step-by-step proofUser code: block = qd.root.pointer(qd.i, 64).dynamic(qd.i, 1024)
a = qd.field(qd.f32); block.place(a)
@qd.kernel
def k():
with qd.stream_parallel():
for i in range(64):
qd.deactivate(block, [i])
with qd.stream_parallel():
for j in range(N):
b[j] = compute_b(j)
The two stream_parallel blocks run strictly sequentially with host syncs between them. Zero concurrency, no error, no warning. Why nothing upstream catches this
Distinct from existing bugs in this PR
FixTwo minimal one-line changes: (a) IR-level, in auto gc_task = Stmt::make_typed<OffloadedStmt>(OffloadedStmt::TaskType::gc, config.arch, kernel);
gc_task->snode = snode;
auto *prev = b->statements[i]->cast<OffloadedStmt>();
if (prev) gc_task->stream_parallel_group_id = prev->stream_parallel_group_id;
b->insert(std::move(gc_task), i + 1);(b) Codegen-level, in each of the 3 sub-blocks of current_task->grid_dim = compile_config.saturating_grid_dim;
current_task->block_dim = 64;
current_task->stream_parallel_group_id = stmt->stream_parallel_group_id; // add
offloaded_tasks.push_back(*current_task);With both in place, |
||
| QD_ASSERT(current_task->grid_dim != 0); | ||
| QD_ASSERT(current_task->block_dim != 0); | ||
| // Host-side adstack sizing. For non-range_for and for const-bound range_for the launcher uses | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Not clear what
itemsis.