refactor(codegen): Consolidate PTO codegen helpers by arity#253
refactor(codegen): Consolidate PTO codegen helpers by arity#253Hzfengsy merged 1 commit intohw-native-sys:mainfrom
Conversation
Summary of ChangesHello @lwDavid, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the 910B PTO codegen helpers by consolidating numerous specialized functions into a smaller set of generic, arity-based helpers. This change aims to eliminate redundant code and simplify the overall code generation process, making it more modular and easier to maintain. The core idea is to abstract away the common patterns of operations based on the number of arguments they take, rather than their specific functionality. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactored PTO code generation in 910B backend by consolidating specialized operation handlers (unary, binary, ternary, etc.) into generalized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 is a great refactoring that consolidates 13 operation-specific codegen helpers into 5 generic arity-based helpers. This significantly reduces code duplication and improves maintainability. The new generic helpers also provide more informative error messages by including the operation name. The changes are clean and correctly applied across all operator registrations. I have one minor suggestion to improve a confusing error message in the MakeFullCodegenPTO function.
| CHECK(op->args_.size() == 2) << "full op requires 3 arguments." | ||
| << op->args_.size(); // Actually 2 args, two of them are conbined! |
There was a problem hiding this comment.
The CHECK message here is incorrect and confusing. It states that 3 arguments are required, but the check is for 2. The appended argument size and the comment add to the confusion. It would be clearer to correct the message and remove the redundant parts.
CHECK(op->args_.size() == 2) << "full op requires 2 arguments.";There was a problem hiding this comment.
This problem fundamentally stems from the mismatch between the number of parameters accepted by the pl-side operator and the number of parameters actually received by the PTO backend.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/910B_PTO/backend_910b_pto_ops.cpp`:
- Around line 133-142: The CHECK in MakeFullCodegenPTO currently asserts
op->args_.size() == 2 but logs "full op requires 3 arguments."; update the
message to reflect the actual expectation (2 arguments) and clarify the
signature (e.g., "full op requires 2 arguments (shape, value), got: ") so the
CHECK for MakeFullCodegenPTO / function MakeFullCodegenPTO and its dynamic_cast
to codegen::PTOCodegen produce an accurate error string when op->args_.size() is
not 2.
- Around line 111-129: Both MakeTileCmpCodegenPTO and MakeTileCvtCodegenPTO use
mode to index cmp_modes.at(mode) and round_modes.at(mode) without validating
mode; add an explicit bounds check before calling .at() (e.g., ensure 0 <= mode
&& mode < cmp_modes.size()/round_modes.size()) and produce a clear CHECK/throw
message identifying the invalid mode and function name, then proceed to build
config_attr and call codegen.Emit with GenerateInsOutsClause as before (also
confirm the attribute concatenation produced by GenerateInsOutsClause matches
expected MLIR syntax).
| // Helper function for full op | ||
| static std::string MakeFullCodegenPTO(const std::string& pto_op_name, const CallPtr& op, | ||
| codegen::CodegenBase& codegen_base) { | ||
| auto& codegen = dynamic_cast<codegen::PTOCodegen&>(codegen_base); | ||
| CHECK(op->args_.size() == 2) << "Binary Axis op requires 2 arguments."; | ||
| codegen.Emit(pto_op_name + " " + GenerateInsOutsClause(op, codegen)); | ||
| CHECK(op->args_.size() == 2) << "full op requires 3 arguments." | ||
| << op->args_.size(); // Actually 2 args, two of them are conbined! | ||
| std::string scalar = codegen.GetExprAsCode(op->args_[1]); | ||
| std::string dst = codegen.GetCurrentResultTarget(); | ||
| codegen.Emit(pto_op_name + " " + "ins(" + scalar + ") outs(" + dst + ")"); | ||
| return ""; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 'block\.full' -g '!**/build/**'Repository: hw-native-sys/pypto
Length of output: 5866
🏁 Script executed:
grep -n "args_\[0\]" src/backend/910B_PTO/backend_910b_pto_ops.cppRepository: hw-native-sys/pypto
Length of output: 146
🏁 Script executed:
rg -n 'MakeFullCodegenPTO' -g '!**/build/**'Repository: hw-native-sys/pypto
Length of output: 313
Fix block.full arg-count message.
The CHECK message says "requires 3 arguments" while enforcing exactly 2. The operation signature is (shape, value), where only the value (args_[1]) is used. Update the message to match the actual requirement:
Suggested fix
- CHECK(op->args_.size() == 2) << "full op requires 3 arguments."
- << op->args_.size(); // Actually 2 args, two of them are conbined!
+ CHECK(op->args_.size() == 2)
+ << "full op requires 2 arguments, got " << op->args_.size();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Helper function for full op | |
| static std::string MakeFullCodegenPTO(const std::string& pto_op_name, const CallPtr& op, | |
| codegen::CodegenBase& codegen_base) { | |
| auto& codegen = dynamic_cast<codegen::PTOCodegen&>(codegen_base); | |
| CHECK(op->args_.size() == 2) << "Binary Axis op requires 2 arguments."; | |
| codegen.Emit(pto_op_name + " " + GenerateInsOutsClause(op, codegen)); | |
| CHECK(op->args_.size() == 2) << "full op requires 3 arguments." | |
| << op->args_.size(); // Actually 2 args, two of them are conbined! | |
| std::string scalar = codegen.GetExprAsCode(op->args_[1]); | |
| std::string dst = codegen.GetCurrentResultTarget(); | |
| codegen.Emit(pto_op_name + " " + "ins(" + scalar + ") outs(" + dst + ")"); | |
| return ""; | |
| // Helper function for full op | |
| static std::string MakeFullCodegenPTO(const std::string& pto_op_name, const CallPtr& op, | |
| codegen::CodegenBase& codegen_base) { | |
| auto& codegen = dynamic_cast<codegen::PTOCodegen&>(codegen_base); | |
| CHECK(op->args_.size() == 2) | |
| << "full op requires 2 arguments, got " << op->args_.size(); | |
| std::string scalar = codegen.GetExprAsCode(op->args_[1]); | |
| std::string dst = codegen.GetCurrentResultTarget(); | |
| codegen.Emit(pto_op_name + " " + "ins(" + scalar + ") outs(" + dst + ")"); | |
| return ""; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/910B_PTO/backend_910b_pto_ops.cpp` around lines 133 - 142, The
CHECK in MakeFullCodegenPTO currently asserts op->args_.size() == 2 but logs
"full op requires 3 arguments."; update the message to reflect the actual
expectation (2 arguments) and clarify the signature (e.g., "full op requires 2
arguments (shape, value), got: ") so the CHECK for MakeFullCodegenPTO / function
MakeFullCodegenPTO and its dynamic_cast to codegen::PTOCodegen produce an
accurate error string when op->args_.size() is not 2.
|
@Hzfengsy Request review. |
2 participants
refactor(codegen): consolidate 910B PTO codegen helpers by arity
Replace 13 operation-specific helper functions (MakeBinaryTileTile,
MakeUnaryTile, MakeBinaryTileScalar, MakeBinaryMatmul, etc.) with 5
generic arity-based helpers (MakeUnary/Binary/Ternary/Quaternary/
QuinaryCodegenPTO), eliminating duplicated logic that differed only
in argument count checks.