Fix nonAccel perfconfig #2124
Fix nonAccel perfconfig #2124
Conversation
dhernandez0
requested review from
justinrosner,
mirza-halilcevic and
umangyadav
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug introduced in PR #2088 where non-accelerated GEMM tuning was incorrectly using v4 perfconfig format instead of v3. Non-accelerated perfconfig must remain at v3 due to having only 9 parameters (vs 12 for v4 used by accelerated operations). The fix updates the version string in GeneralGemmParamsAttr from "v4:" to "v3:" and adds comprehensive unit tests to prevent regression.
- Changed GeneralGemmParamsAttr perfconfig format from v4 to v3 in RockAttrDefs.td
- Added unit tests for InitParamsNonAccel deserialization covering positive and negative test cases
- Updated test expectations in emit-tuning-space.mlir to reflect the correct v3 format
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mlir/unittests/Dialect/Rock/InitParamsNonAccelTests.cpp | New test file with comprehensive tests for v3 perfconfig deserialization including positive cases and negative validation tests |
| mlir/unittests/Dialect/Rock/CMakeLists.txt | Added InitParamsNonAccelTests.cpp to the test build configuration |
| mlir/test/rocmlir-gen/emit-tuning-space.mlir | Updated CHECK-NAVI expectation from v4 to v3 format for non-accelerated GEMM on gfx1100 architecture |
| mlir/include/mlir/Dialect/Rock/IR/RockAttrDefs.td | Fixed GeneralGemmParamsAttr::getPerfConfigStr to output "v3:" instead of "v4:" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //===- InitParamsNonAccelTests.cpp - Tests for InitParamsNonAccel | ||
| //--------------===// |
There was a problem hiding this comment.
The comment header is split across two lines which is inconsistent with the formatting used in InitParamsAccelTests.cpp. The entire header should be on a single line like:
//===- InitParamsNonAccelTests.cpp - Tests for InitParamsNonAccel -------===//| //===- InitParamsNonAccelTests.cpp - Tests for InitParamsNonAccel | |
| //--------------===// | |
| //===- InitParamsNonAccelTests.cpp - Tests for InitParamsNonAccel -------===// |
Motivation
A bug was introduced in #2088 that makes non-accel tuning fail. Non accel perfConfig has to stay in v3 for now due to the number of params.
Technical Details
Add tests for nonaccel perfConfig and change it back to v3.
Test Plan
All tests pass.
Test Result
All tests pass.
Submission Checklist