[PTO][IR] align textract tinsert and tquant advanced parameters#729
[PTO][IR] align textract tinsert and tquant advanced parameters#729FangRui0 wants to merge 1 commit into
Conversation
Codex Review该评论由 review 机器人自动更新。
Summary发现 3 个问题： Findings
本 PR 在
PR 文档和新增测试都把
这里把扩展 stats 家族是否存在定义成 |
|
/run a3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
![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 extends the PTO dialect's extraction, insertion, and quantization operations (textract, tinsert, and tquant) to support advanced parameters and attributes for the A5 architecture, updating their definitions, verifiers, and EmitC/Memref lowering passes. Feedback on the changes highlights a critical compilation error in lib/PTO/IR/PTO.cpp where mode->getValue() is incorrectly called on an optional enum, a parsing ambiguity in the declarative assembly format of TQuantOp due to sequential optional operands, and code duplication of vector pre-quant type pair helpers.
| if (auto mode = getAccToVecMode(); | ||
| hasMode && mode && mode->getValue() != pto::AccToVecMode::SingleModeVec0 && | ||
| mode->getValue() != pto::AccToVecMode::SingleModeVec1) |
There was a problem hiding this comment.
The getAccToVecMode() method returns std::optional<pto::AccToVecMode>. Since pto::AccToVecMode is an enum class, it does not have a getValue() member function. Calling mode->getValue() will result in a compilation error. Instead, you should dereference the optional using *mode or call mode.value().
Note that this same issue occurs at multiple other places in this file:
- Line 5627-5629
- Line 5760-5762
- Line 5812-5815
- Line 5989-5993
- Line 6023-6027
- Line 6098-6102
- Line 6129-6133
Please fix all of them similarly.
| if (auto mode = getAccToVecMode(); | |
| hasMode && mode && mode->getValue() != pto::AccToVecMode::SingleModeVec0 && | |
| mode->getValue() != pto::AccToVecMode::SingleModeVec1) | |
| if (auto mode = getAccToVecMode(); | |
| hasMode && mode && *mode != pto::AccToVecMode::SingleModeVec0 && | |
| *mode != pto::AccToVecMode::SingleModeVec1) |
| let assemblyFormat = [{ | ||
| `ins` `(` $src `,` $fp (`,` $offset^)? `:` qualified(type($src)) `,` qualified(type($fp)) (`,` qualified(type($offset))^)? `)` | ||
| `ins` `(` $src | ||
| (`,` $fp^)? (`,` $offset^)? (`,` $exp^)? (`,` $max^)? (`,` $scaling^)? (`,` $expZZ^)? | ||
| `:` qualified(type($src)) | ||
| (`,` qualified(type($fp))^)? (`,` qualified(type($offset))^)? (`,` qualified(type($exp))^)? | ||
| (`,` qualified(type($max))^)? (`,` qualified(type($scaling))^)? (`,` qualified(type($expZZ))^)? | ||
| `)` | ||
| `outs` `(` $dst `:` qualified(type($dst)) `)` | ||
| attr-dict | ||
| }]; |
There was a problem hiding this comment.
The declarative assembly format for TQuantOp has multiple optional operands ($fp, $offset, $exp, $max, $scaling, $expZZ) parsed sequentially without any distinguishing keywords or markers.
Because the parser cannot distinguish SSA values by name, if any of the leading optional operands are omitted (for example, in the A5 form where fp and offset are omitted but exp, max, and scaling are provided), the parser will sequentially assign the first parsed optional operand to $fp, the second to $offset, and so on. This results in completely incorrect operand mapping and silent correctness bugs.
To resolve this, you should either:
- Define a custom parser and printer (
let hasCustomAssemblyFormat = 1;) to handle the two mutually exclusive forms unambiguously. - Or introduce distinguishing keywords in the declarative assembly format, e.g.,
(,$fp^)? (,$offset^)?etc.
| auto isA2A3VectorPreQuantTypePairLocal = [&](Type srcElem, Type dstElem) { | ||
| if (srcElem.isF32()) | ||
| return dstElem.isInteger(8); | ||
| if (srcElem.isInteger(32)) | ||
| return dstElem.isInteger(8) || dstElem.isF16() || dstElem.isInteger(16); | ||
| return false; | ||
| }; | ||
| auto isA5VectorPreQuantTypePairLocal = [&](Type srcElem, Type dstElem) { | ||
| if (srcElem.isF32()) | ||
| return dstElem.isInteger(8) || | ||
| (isa<FloatType>(dstElem) && cast<FloatType>(dstElem).getWidth() == 8) || | ||
| dstElem.isF16() || dstElem.isBF16() || dstElem.isF32(); | ||
| if (srcElem.isInteger(32)) | ||
| return dstElem.isInteger(8) || dstElem.isF16() || dstElem.isBF16(); | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
These local lambdas (isA2A3VectorPreQuantTypePairLocal and isA5VectorPreQuantTypePairLocal) duplicate the logic of the global helper functions isA2A3VectorPreQuantTypePair and isA5VectorPreQuantTypePair declared at lines 2991-2992 and used elsewhere (e.g., in TInsertOp::verify()).
To improve maintainability and eliminate code duplication, please remove these local lambdas and use the global helper functions directly in TExtractOp::verify().
// Using global helper functions instead of local lambdas
A3 板测失败
日志尾部 |
|
/run a3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A3 板测失败
日志尾部 |
2 participants
No description provided.