Open
Conversation
Contributor
Author
|
/run a5 |
![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 introduces the pto.tgemv.mx operation family for mixed-precision matrix-vector multiplication on A5 targets, including IR definitions, documentation, and lowering patterns. Review feedback highlights the need for unique intrinsic names for the accumulation and bias variants to prevent backend collisions, the importance of updating conversion patterns and test expectations accordingly, and the necessity of preserving result types during view-to-memref transformations to maintain SSA consistency.
| }]; | ||
|
|
||
| let extraClassDeclaration = [{ | ||
| static StringRef getIntrinsicName() { return "TGEMV_MX"; } |
There was a problem hiding this comment.
The intrinsic name for the accumulation variant should be specific to avoid collisions and ensure correct mapping in the backend. Following the pattern of other operations (like TMatmulMXAccOp), this should be TGEMV_MX_ACC.
static StringRef getIntrinsicName() { return "TGEMV_MX_ACC"; }
Contributor
Author
There was a problem hiding this comment.
Backend API named "TGEMV_MX".
| }]; | ||
|
|
||
| let extraClassDeclaration = [{ | ||
| static StringRef getIntrinsicName() { return "TGEMV_MX"; } |
There was a problem hiding this comment.
The intrinsic name for the bias variant should be specific. Using the same name as the base operation (TGEMV_MX) will cause issues during code generation as the argument count and positions differ. It should be TGEMV_MX_BIAS.
static StringRef getIntrinsicName() { return "TGEMV_MX_BIAS"; }
Contributor
Author
There was a problem hiding this comment.
Backend API named "TGEMV_MX".
lib/PTO/Transforms/PTOToEmitC.cpp
Outdated
Comment on lines
+6884
to
+6885
| replaceOrEraseWithOpaqueCall(op.getOperation(), "TGEMV_MX", | ||
| {dst, cIn, a, aScale, b, bScale}, rewriter); |
There was a problem hiding this comment.
This variant should map to the TGEMV_MX_ACC intrinsic. Mapping both acc and bias variants to the same TGEMV_MX name with different argument structures will lead to incorrect C code generation or compilation errors in the backend.
Suggested change
| replaceOrEraseWithOpaqueCall(op.getOperation(), "TGEMV_MX", | |
| {dst, cIn, a, aScale, b, bScale}, rewriter); | |
| replaceOrEraseWithOpaqueCall(op.getOperation(), "TGEMV_MX_ACC", | |
| {dst, cIn, a, aScale, b, bScale}, rewriter); |
Contributor
Author
There was a problem hiding this comment.
Backend API named "TGEMV_MX".
lib/PTO/Transforms/PTOToEmitC.cpp
Outdated
Comment on lines
+6903
to
+6904
| replaceOrEraseWithOpaqueCall(op.getOperation(), "TGEMV_MX", | ||
| {dst, a, aScale, b, bScale, bias}, rewriter); |
There was a problem hiding this comment.
This variant should map to the TGEMV_MX_BIAS intrinsic. Using a generic name for operations with different argument signatures is fragile and likely incorrect for the EmitC backend.
Suggested change
| replaceOrEraseWithOpaqueCall(op.getOperation(), "TGEMV_MX", | |
| {dst, a, aScale, b, bScale, bias}, rewriter); | |
| replaceOrEraseWithOpaqueCall(op.getOperation(), "TGEMV_MX_BIAS", | |
| {dst, a, aScale, b, bScale, bias}, rewriter); |
Contributor
Author
There was a problem hiding this comment.
Backend API named "TGEMV_MX".
| } | ||
| }; | ||
|
|
||
| struct PTOTGemvMXAccToTGEMV_MX |
There was a problem hiding this comment.
The pattern name should reflect the specific intrinsic it targets. It is recommended to rename this to PTOTGemvMXAccToTGEMV_MX_ACC for consistency with other mixed-precision accumulation patterns.
Suggested change
| struct PTOTGemvMXAccToTGEMV_MX | |
| struct PTOTGemvMXAccToTGEMV_MX_ACC |
| } | ||
| }; | ||
|
|
||
| struct PTOTGemvMXBiasToTGEMV_MX |
There was a problem hiding this comment.
The pattern name should be updated to PTOTGemvMXBiasToTGEMV_MX_BIAS to match the intended intrinsic target.
Suggested change
| struct PTOTGemvMXBiasToTGEMV_MX | |
| struct PTOTGemvMXBiasToTGEMV_MX_BIAS |
| IRRewriter rewriter(ctx); | ||
| rewriter.setInsertionPoint(op); | ||
| rewriter.replaceOpWithNewOp<pto::TGemvMxOp>( | ||
| op, TypeRange{}, |
There was a problem hiding this comment.
Using TypeRange{} will drop any results the original operation might have had. If these operations are in the tensor domain (where they return a result aliasing the destination buffer), this will break the SSA chain. It is safer to use op->getResultTypes() to preserve the original result signature.
Suggested change
| op, TypeRange{}, | |
| op, op->getResultTypes(), |
Comment on lines
+22
to
+23
| // CHECK: TGEMV_MX( | ||
| // CHECK: TGEMV_MX( |
There was a problem hiding this comment.
The test expectations should be updated to reflect the specific intrinsic names for the accumulation and bias variants.
// CHECK: TGEMV_MX_ACC(
// CHECK: TGEMV_MX_BIAS(
A5 板测失败
日志尾部 |
Contributor
Author
|
/run a5 |
A5 板测失败
日志尾部 |
Contributor
Author
|
/run a3 |
Contributor
Author
|
/run a5 |
Contributor
Author
|
/run a5 test/basic/tgemv_mx_emitc.pto |
A5 板测失败
失败用例
|
A5 板测失败详情：PR #418tgemv_mx_emitc
|
Contributor
Author
|
/run a5 test/basic/tgemv_mx_emitc.pto test/basic/tgemv_mx_variants_emitc.pto |
A5 板测失败
日志尾部 |
1 similar comment
A5 板测失败
日志尾部 |
Contributor
Author
|
/run a5 tgemv_mx_emitc tgemv_mx_variants_emitc |
A5 板测成功
|
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
4 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.
TGEMV_MX is A5-only op