Add comm engine attrs for collective and async ops#723
Conversation
Codex Review该评论由 review 机器人自动更新。
Summary检查到 2 个 P2 问题:新增 engine attrs 引入了生成 builder 的兼容性回归,而且 async session/transfer 的 engine 一致性没有被 verifier 约束。 Findings
这个 PR 让 |
There was a problem hiding this comment.
Code Review
This pull request introduces DMA and collective communication engine attributes (dmaEngine and collEngine) to the PTO dialect, updating TableGen definitions, C API exports, Python bindings, and the EmitC lowering pass to support selecting different engines (e.g., SDMA vs. URMA, and AIV vs. CCU). The review feedback highlights critical issues regarding Python bindings and API backward compatibility. Specifically, using MlirContext directly in the Python binding lambdas instead of DefaultingPyMlirContext can lead to segmentation faults when the context is omitted. Additionally, inserting the new collEngine attribute before existing attributes like root and reduceOp in TableGen arguments breaks backward compatibility for positional arguments in both C++ and Python APIs; these new default-valued attributes should be appended to the end of the argument lists.
| [](py::object cls, mlir::pto::DmaEngine value, MlirContext ctx) -> py::object { | ||
| MlirAttribute a = mlirPTODmaEngineAttrGet(ctx, static_cast<int32_t>(value)); |
There was a problem hiding this comment.
Using MlirContext directly in the Python binding lambda prevents the context from being optional (defaulting to the active context in the thread-local stack). If the context is omitted or passed as None in Python, this will either raise a TypeError or pass a null context to mlirPTODmaEngineAttrGet, leading to a segmentation fault when unwrap(ctx) is called. Use DefaultingPyMlirContext instead, which automatically resolves to the default context.
| [](py::object cls, mlir::pto::DmaEngine value, MlirContext ctx) -> py::object { | |
| MlirAttribute a = mlirPTODmaEngineAttrGet(ctx, static_cast<int32_t>(value)); | |
| [](py::object cls, mlir::pto::DmaEngine value, DefaultingPyMlirContext ctx) -> py::object { | |
| MlirAttribute a = mlirPTODmaEngineAttrGet(ctx.get(), static_cast<int32_t>(value)); |
| [](py::object cls, mlir::pto::CollEngine value, MlirContext ctx) -> py::object { | ||
| MlirAttribute a = mlirPTOCollEngineAttrGet(ctx, static_cast<int32_t>(value)); |
There was a problem hiding this comment.
Using MlirContext directly in the Python binding lambda prevents the context from being optional. If the context is omitted or passed as None in Python, this will either raise a TypeError or pass a null context to mlirPTOCollEngineAttrGet, leading to a segmentation fault when unwrap(ctx) is called. Use DefaultingPyMlirContext instead, which automatically resolves to the default context.
| [](py::object cls, mlir::pto::CollEngine value, MlirContext ctx) -> py::object { | |
| MlirAttribute a = mlirPTOCollEngineAttrGet(ctx, static_cast<int32_t>(value)); | |
| [](py::object cls, mlir::pto::CollEngine value, DefaultingPyMlirContext ctx) -> py::object { | |
| MlirAttribute a = mlirPTOCollEngineAttrGet(ctx.get(), static_cast<int32_t>(value)); |
| DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine, | ||
| I32Attr:$root |
There was a problem hiding this comment.
Inserting the new collEngine attribute before the existing root attribute in the TableGen arguments list breaks backward compatibility for both C++ and Python APIs. Any existing code calling TBroadcastOp with positional arguments will now have their arguments mismatched (e.g., passing the root rank to collEngine). Placing new optional or default-valued attributes at the end of the arguments list preserves the positional argument order and maintains backward compatibility.
I32Attr:$root,
DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine
| DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine, | ||
| I32Attr:$root |
There was a problem hiding this comment.
Inserting the new collEngine attribute before the existing root attribute in the TableGen arguments list breaks backward compatibility for both C++ and Python APIs. Placing new optional or default-valued attributes at the end of the arguments list preserves the positional argument order and maintains backward compatibility.
I32Attr:$root,
DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine
| DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine, | ||
| I32Attr:$root |
There was a problem hiding this comment.
Inserting the new collEngine attribute before the existing root attribute in the TableGen arguments list breaks backward compatibility for both C++ and Python APIs. Placing new optional or default-valued attributes at the end of the arguments list preserves the positional argument order and maintains backward compatibility.
I32Attr:$root,
DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine
| DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine, | ||
| PTO_ReduceOpAttr:$reduceOp, | ||
| I32Attr:$root |
There was a problem hiding this comment.
Inserting the new collEngine attribute before the existing reduceOp and root attributes in the TableGen arguments list breaks backward compatibility for both C++ and Python APIs. Placing new optional or default-valued attributes at the end of the arguments list preserves the positional argument order and maintains backward compatibility.
PTO_ReduceOpAttr:$reduceOp,
I32Attr:$root,
DefaultValuedAttr<PTO_CollEngineAttr, "::mlir::pto::CollEngine::AIV">:$collEngine
No description provided.