feat: add dynamic buf_id support for pto.get_buf / pto.rls_buf#905
Open
kuri780 wants to merge 4 commits into
Open
feat: add dynamic buf_id support for pto.get_buf / pto.rls_buf#905kuri780 wants to merge 4 commits into
kuri780 wants to merge 4 commits into
Conversation
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for dynamic buffer-ID synchronization in SIMT kernels by updating pto.get_buf and pto.rls_buf to accept an optional dynamic SSA value in addition to the static attribute. The changes span the ODS definitions, parser, printer, verifier, EmitC conversion, LLVM emitters, and Python DSL, and include a new SIMT test case. The review feedback focuses on refactoring opportunities to eliminate code duplication in the parser and printer, removing redundant Attribute casts, and improving efficiency by avoiding unnecessary StringAttr lookups in the LLVM emitters.
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.
Comment on lines
7802
to
7804
| static ParseResult parseBufSyncOp(OpAsmParser &parser, OperationState &result) { | ||
| Attribute opTypeAttr; | ||
| IntegerAttr bufIdAttr; | ||
| IntegerAttr modeAttr; |
Contributor
There was a problem hiding this comment.
To avoid duplicating the 15-line parsing logic for the optional dynamic/static buffer ID, we can extract it into a helper lambda at the beginning of parseBufSyncOp.
static ParseResult parseBufSyncOp(OpAsmParser &parser, OperationState &result) {
Attribute opTypeAttr;
IntegerAttr modeAttr;
auto parseBufId = [&]() -> ParseResult {
OpAsmParser::UnresolvedOperand bufOperand;
OptionalParseResult parseBufOperand =
parser.parseOptionalOperand(bufOperand);
if (parseBufOperand.has_value()) {
if (failed(*parseBufOperand))
return failure();
return parser.resolveOperand(bufOperand,
parser.getBuilder().getIndexType(),
result.operands);
}
IntegerAttr bufIdAttr;
if (parseI32LiteralAttr(parser, bufIdAttr))
return failure();
result.addAttribute("buf_id", bufIdAttr);
return success();
};
Comment on lines
+7819
to
+7834
| OpAsmParser::UnresolvedOperand bufOperand; | ||
| OptionalParseResult parseBufOperand = | ||
| parser.parseOptionalOperand(bufOperand); | ||
| if (parseBufOperand.has_value()) { | ||
| if (failed(*parseBufOperand)) | ||
| return failure(); | ||
| if (parser.resolveOperand(bufOperand, | ||
| parser.getBuilder().getIndexType(), | ||
| result.operands)) | ||
| return failure(); | ||
| } else { | ||
| IntegerAttr bufIdAttr; | ||
| if (parseI32LiteralAttr(parser, bufIdAttr)) | ||
| return failure(); | ||
| result.addAttribute("buf_id", bufIdAttr); | ||
| } |
Contributor
There was a problem hiding this comment.
Use the helper lambda parseBufId here to simplify the parsing logic and eliminate code duplication.
Suggested change
| OpAsmParser::UnresolvedOperand bufOperand; | |
| OptionalParseResult parseBufOperand = | |
| parser.parseOptionalOperand(bufOperand); | |
| if (parseBufOperand.has_value()) { | |
| if (failed(*parseBufOperand)) | |
| return failure(); | |
| if (parser.resolveOperand(bufOperand, | |
| parser.getBuilder().getIndexType(), | |
| result.operands)) | |
| return failure(); | |
| } else { | |
| IntegerAttr bufIdAttr; | |
| if (parseI32LiteralAttr(parser, bufIdAttr)) | |
| return failure(); | |
| result.addAttribute("buf_id", bufIdAttr); | |
| } | |
| if (failed(parseBufId())) | |
| return failure(); |
Comment on lines
+7846
to
+7861
| OpAsmParser::UnresolvedOperand bufOperand; | ||
| OptionalParseResult parseBufOperand = | ||
| parser.parseOptionalOperand(bufOperand); | ||
| if (parseBufOperand.has_value()) { | ||
| if (failed(*parseBufOperand)) | ||
| return failure(); | ||
| if (parser.resolveOperand(bufOperand, | ||
| parser.getBuilder().getIndexType(), | ||
| result.operands)) | ||
| return failure(); | ||
| } else { | ||
| IntegerAttr bufIdAttr; | ||
| if (parseI32LiteralAttr(parser, bufIdAttr)) | ||
| return failure(); | ||
| result.addAttribute("buf_id", bufIdAttr); | ||
| } |
Contributor
There was a problem hiding this comment.
Use the helper lambda parseBufId here to simplify the parsing logic and eliminate code duplication.
Suggested change
| OpAsmParser::UnresolvedOperand bufOperand; | |
| OptionalParseResult parseBufOperand = | |
| parser.parseOptionalOperand(bufOperand); | |
| if (parseBufOperand.has_value()) { | |
| if (failed(*parseBufOperand)) | |
| return failure(); | |
| if (parser.resolveOperand(bufOperand, | |
| parser.getBuilder().getIndexType(), | |
| result.operands)) | |
| return failure(); | |
| } else { | |
| IntegerAttr bufIdAttr; | |
| if (parseI32LiteralAttr(parser, bufIdAttr)) | |
| return failure(); | |
| result.addAttribute("buf_id", bufIdAttr); | |
| } | |
| if (failed(parseBufId())) | |
| return failure(); |
Comment on lines
7886
to
7914
| if (auto pipeAttr = dyn_cast<PipeAttr>(opTypeAttr)) { | ||
| p << " \"" << stringifyPIPE(pipeAttr.getPipe()) << "\", " | ||
| << bufIdAttr.getInt() << ", " << modeAttr.getInt(); | ||
| p << " \"" << stringifyPIPE(pipeAttr.getPipe()) << "\", "; | ||
| if (bufIdAttr) | ||
| p << bufIdAttr.getInt(); | ||
| else | ||
| p << bufIdDyn; | ||
| p << ", " << modeAttr.getInt(); | ||
| } else if (auto pipeEventType = dyn_cast<PipeEventTypeAttr>(opTypeAttr)) { | ||
| p << "[" << opTypeAttr << ", " << bufIdAttr.getInt() << ", " | ||
| << modeAttr.getInt() << "]"; | ||
| p << "[" << opTypeAttr << ", "; | ||
| if (bufIdAttr) | ||
| p << bufIdAttr.getInt(); | ||
| else | ||
| p << bufIdDyn; | ||
| p << ", " << modeAttr.getInt() << "]"; | ||
| } else if (auto syncOpType = dyn_cast<SyncOpTypeAttr>(opTypeAttr)) { | ||
| p << "[" << opTypeAttr << ", " << bufIdAttr.getInt() << ", " | ||
| << modeAttr.getInt() << "]"; | ||
| p << "[" << opTypeAttr << ", "; | ||
| if (bufIdAttr) | ||
| p << bufIdAttr.getInt(); | ||
| else | ||
| p << bufIdDyn; | ||
| p << ", " << modeAttr.getInt() << "]"; | ||
| } else { | ||
| p << "[" << opTypeAttr << ", " << bufIdAttr.getInt() << ", " | ||
| << modeAttr.getInt() << "]"; | ||
| p << "[" << opTypeAttr << ", "; | ||
| if (bufIdAttr) | ||
| p << bufIdAttr.getInt(); | ||
| else | ||
| p << bufIdDyn; | ||
| p << ", " << modeAttr.getInt() << "]"; | ||
| } |
Contributor
There was a problem hiding this comment.
The printing of the buffer ID and mode is duplicated across all branches of the if-else chain. We can simplify this by factoring out the common printing logic.
if (auto pipeAttr = dyn_cast<PipeAttr>(opTypeAttr)) {
p << " \"" << stringifyPIPE(pipeAttr.getPipe()) << "\", ";
} else {
p << "[" << opTypeAttr << ", ";
}
if (bufIdAttr)
p << bufIdAttr.getInt();
else
p << bufIdDyn;
p << ", " << modeAttr.getInt();
if (!isa<PipeAttr>(opTypeAttr))
p << "]";
Comment on lines
+91
to
+94
| rewriter.create<pto::GetBufOp>( | ||
| op->getLoc(), Attribute(opTypeAttr), | ||
| rewriter.getI32IntegerAttr(static_cast<int32_t>(physicalId)), | ||
| rewriter.getI32IntegerAttr(0), Value()); |
Contributor
There was a problem hiding this comment.
The cast Attribute(opTypeAttr) is redundant because opTypeAttr is already declared as an Attribute.
Suggested change
| rewriter.create<pto::GetBufOp>( | |
| op->getLoc(), Attribute(opTypeAttr), | |
| rewriter.getI32IntegerAttr(static_cast<int32_t>(physicalId)), | |
| rewriter.getI32IntegerAttr(0), Value()); | |
| rewriter.create<pto::GetBufOp>( | |
| op->getLoc(), opTypeAttr, | |
| rewriter.getI32IntegerAttr(static_cast<int32_t>(physicalId)), | |
| rewriter.getI32IntegerAttr(0), Value()); |
Comment on lines
+112
to
+115
| rewriter.create<pto::RlsBufOp>( | ||
| op->getLoc(), Attribute(opTypeAttr), | ||
| rewriter.getI32IntegerAttr(static_cast<int32_t>(physicalId)), | ||
| rewriter.getI32IntegerAttr(0), Value()); |
Contributor
There was a problem hiding this comment.
The cast Attribute(opTypeAttr) is redundant because opTypeAttr is already declared as an Attribute.
Suggested change
| rewriter.create<pto::RlsBufOp>( | |
| op->getLoc(), Attribute(opTypeAttr), | |
| rewriter.getI32IntegerAttr(static_cast<int32_t>(physicalId)), | |
| rewriter.getI32IntegerAttr(0), Value()); | |
| rewriter.create<pto::RlsBufOp>( | |
| op->getLoc(), opTypeAttr, | |
| rewriter.getI32IntegerAttr(static_cast<int32_t>(physicalId)), | |
| rewriter.getI32IntegerAttr(0), Value()); |
Comment on lines
+8663
to
+8668
| if constexpr (std::is_same_v<BufSyncOp, pto::GetBufOp>) | ||
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.GET.BUF.mode") | ||
| .getValue(); | ||
| else | ||
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.RLS.BUF.mode") | ||
| .getValue(); |
Contributor
There was a problem hiding this comment.
Using StringAttr::get(...).getValue() is inefficient because it performs a lookup/insertion in the MLIR context's hash map. Since calleeName is a StringRef, you can assign string literals directly.
Suggested change
| if constexpr (std::is_same_v<BufSyncOp, pto::GetBufOp>) | |
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.GET.BUF.mode") | |
| .getValue(); | |
| else | |
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.RLS.BUF.mode") | |
| .getValue(); | |
| if constexpr (std::is_same_v<BufSyncOp, pto::GetBufOp>) | |
| calleeName = "llvm.hivm.GET.BUF.mode"; | |
| else | |
| calleeName = "llvm.hivm.RLS.BUF.mode"; |
Comment on lines
+8607
to
+8612
| if constexpr (std::is_same_v<BufSyncOp, pto::GetBufOp>) | ||
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.GET.BUF.mode") | ||
| .getValue(); | ||
| else | ||
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.RLS.BUF.mode") | ||
| .getValue(); |
Contributor
There was a problem hiding this comment.
Using StringAttr::get(...).getValue() is inefficient because it performs a lookup/insertion in the MLIR context's hash map. Since calleeName is a StringRef, you can assign string literals directly.
Suggested change
| if constexpr (std::is_same_v<BufSyncOp, pto::GetBufOp>) | |
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.GET.BUF.mode") | |
| .getValue(); | |
| else | |
| calleeName = StringAttr::get(op.getContext(), "llvm.hivm.RLS.BUF.mode") | |
| .getValue(); | |
| if constexpr (std::is_same_v<BufSyncOp, pto::GetBufOp>) | |
| calleeName = "llvm.hivm.GET.BUF.mode"; | |
| else | |
| calleeName = "llvm.hivm.RLS.BUF.mode"; |
KurrinQu
reviewed
Jul 7, 2026
| // Safe to load MTE result from UB | ||
| ``` | ||
|
|
||
| ### Buffer-ID Synchronization (`pto.get_buf` / `pto.rls_buf`) |
| PTO_PipeLikeAttr:$op_type, | ||
| I32Attr:$buf_id, | ||
| DefaultValuedAttr<I32Attr, "0">:$mode | ||
| OptionalAttr<I32Attr>:$buf_id, |
Contributor
There was a problem hiding this comment.
我建议是新增GetBufDynOp，参考WaitFlagOp和WaitFlagDynOp的形式。整体更清楚一些。
Allow buf_id to be provided as a dynamic SSA value (index type) in addition to the existing static integer attribute. The two forms are mutually exclusive — exactly one must be provided. This enables SIMT double-buffering patterns where buf_id is computed at runtime, e.g. (iter & 1) for ping-pong buffering. Changes across layers: - ODS (PTOOps.td): buf_id attr → OptionalAttr, add buf_id_dyn operand - IR (PTO.cpp): verifier enforces mutual exclusion, custom assembly format handles both static/dynamic forms - VPTO LLVM emitter: static→GET.BUFI.mode (immediate), dynamic→ GET.BUF.mode (register) - CANN900 LLVM emitter: same pattern - EmitC: static passes attr, dynamic passes via operands - BufidSync pass: adapted to new constructor signature - Python DSL: get_buf/rls_buf accept int or SSA value for buf_id - Docs (17-simt.md): documented barrier, get_buf/rls_buf with both static and dynamic syntax Test: simt-bufid-dynamic-core exercises dynamic buf_id from scf.for loop induction variable with ping-pong pattern, verified on simulator (compare passed).
Split GetBufOp/RlsBufOp into separate static and dynamic variants, following the WaitFlagOp / WaitFlagDynOp pattern: - GetBufOp / RlsBufOp: static buf_id (I32Attr), used by BufidSync pass - GetBufDynOp / RlsBufDynOp: dynamic buf_id (Index operand), for runtime-computed patterns like SIMT ping-pong buffering This replaces the previous approach of a single op with OptionalAttr + Optional operand and a mutual-exclusion verifier. Each op now has a clean, focused definition without branching in consumers. Changes across layers: - ODS (PTOOps.td): revert GetBufOp/RlsBufOp to static-only, add GetBufDynOp/RlsBufDynOp with hasCustomAssemblyFormat - IR (PTO.cpp): separate verifiers, parsers, and printers for static and dynamic variants - LLVM emitters (VPTO + CANN900): static→GET.BUFI.mode (immediate), dynamic→GET.BUF.mode (register) via separate patterns - EmitC: separate patterns for static (attr) and dynamic (operand) buf_id - BufidSync: revert to simpler constructor (static-only) - Python DSL: get_buf/rls_buf for static int, get_buf_dyn/rls_buf_dyn for SSA value - Docs: move dynamic buf_id docs from 17-simt.md to 01-pipeline-sync.md, keep brief reference in SIMT doc - Test: update kernel.pto to use get_buf_dyn/rls_buf_dyn Co-Authored-By: Claude <noreply@anthropic.com>
The dynamic variants of get_buf/rls_buf were missing from the addIllegalOp list in configureVPTOOpLoweringTarget(). Their lowering patterns (LowerBufDynSyncOpPattern) were registered but never fired, leaving index-typed buf_id operands unconverted. The type conversion framework then injected unrealized_conversion_cast ops that failed LLVM translation. Add pto::GetBufDynOp and pto::RlsBufDynOp to the illegal op list in both VPTOLLVMEmitter and VPTOCANN900LLVMEmitter, consistent with the existing static GetBufOp/RlsBufOp entries. Co-Authored-By: Claude <noreply@anthropic.com>
The syncthreads memory ordering contract, pto.barrier section, and dynamic buf_id cross-reference note are independent documentation improvements that do not belong to the dynamic buf_id feature. They should be submitted as a separate docs-only PR. Co-Authored-By: Claude <noreply@anthropic.com>
kuri780
force-pushed
the
feature/simt-sync-dynamic-bufid
branch
from
107e2b8 to
94964da
Compare
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
3 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.
Allow buf_id to be provided as a dynamic SSA value (index type) in addition to the existing static integer attribute. The two forms are mutually exclusive — exactly one must be provided.
This enables SIMT double-buffering patterns where buf_id is computed at runtime, e.g. (iter & 1) for ping-pong buffering.
Changes across layers:
Test: simt-bufid-dynamic-core exercises dynamic buf_id from scf.for loop induction variable with ping-pong pattern, verified on simulator (compare passed).