fix(codegen): Correct config_attr position and cmp_modes case in PTO ops#273
fix(codegen): Correct config_attr position and cmp_modes case in PTO ops#273Hzfengsy 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 refines the code generation for PTO operations by addressing two specific syntax and specification compliance issues. It ensures that comparison modes are correctly cased for PTO assembly and that configuration attributes are placed appropriately within the generated MLIR code, thereby improving the overall correctness and compatibility of the backend output. Highlights
Changelog
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
|
📝 WalkthroughWalkthroughThis PR modifies the backend 910B PTO operations code by converting comparison mode constants to lowercase ("eq", "ne", "lt", etc.) and reordering the emission of config_attr to occur before type annotations instead of after, while removing the duplicate emission block. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
The pull request correctly addresses the stated objectives by lowercasing cmp_modes to align with the PTO assembly specification and repositioning the config_attr output to match the expected MLIR syntax. These changes appear to be necessary for correctness and consistency with external specifications.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 62-64: The code appends config_attr directly to the last operand
via the output stream oss (if (!config_attr.empty()) { oss << config_attr; }),
which can produce malformed assembly; change the append to include an explicit
leading space (e.g., oss << ' ' << config_attr) so the attribute dictionary is
separated from the previous token—update the conditional that checks config_attr
and the oss write to insert the space before config_attr.
| if (!config_attr.empty()) { | ||
| oss << config_attr; | ||
| } |
There was a problem hiding this comment.
Missing separator before config_attr can produce malformed assembly.
At Line 63, config_attr is concatenated directly to the last operand (...%arg{...}), which is brittle and can break parsing depending on grammar/tokenization. Add an explicit leading space before the attribute dict.
Proposed fix
- if (!config_attr.empty()) {
- oss << config_attr;
- }
+ if (!config_attr.empty()) {
+ oss << " " << config_attr;
+ }🤖 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 62 - 64, The code
appends config_attr directly to the last operand via the output stream oss (if
(!config_attr.empty()) { oss << config_attr; }), which can produce malformed
assembly; change the append to include an explicit leading space (e.g., oss << '
' << config_attr) so the attribute dictionary is separated from the previous
token—update the conditional that checks config_attr and the oss write to insert
the space before config_attr.
There was a problem hiding this comment.
Referenced the documented grammar implementation here.
There was a problem hiding this comment.
@lwDavid, thank you for verifying against the documented grammar implementation. Since you've checked the PTO assembly specification and confirmed the current syntax is correct, I'll defer to your verification.
If you have a link to the relevant grammar documentation handy, feel free to share it for future reference, but it's not necessary.
|
@Hzfengsy Bug fix. Request review. |
2 participants
fix(codegen): Correct config_attr position and cmp_modes case in PTO ops
Move config_attr output before type annotations to match expected MLIR
syntax. Lowercase cmp_modes to align with PTO assembly specification.