Skip to content

Conversation

@dhernandez0
Copy link
Contributor

@dhernandez0 dhernandez0 commented Mar 24, 2025

This PR depends on #1774, that PR needs to be merged first.

In this PR we add migraphx integration and end-to-end tests in migraphx dialect. The end-to-end tests are based on attention tests.

TODO:

@dhernandez0 dhernandez0 self-assigned this Mar 24, 2025
@dhernandez0 dhernandez0 requested a review from causten as a code owner March 24, 2025 16:10
@dhernandez0 dhernandez0 changed the base branch from develop to 1704-gemm-elementwise-gemm March 24, 2025 16:11
@dhernandez0 dhernandez0 force-pushed the 1704-gemm-elementwise-gemm branch 2 times, most recently from bd2c24e to 54237b0 Compare March 26, 2025 07:53
@dhernandez0 dhernandez0 force-pushed the gemm_gemm_migraphx_integration branch 2 times, most recently from 2b1e306 to 5fb22e3 Compare March 26, 2025 08:28
@codecov
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 86.94030% with 35 lines in your changes missing coverage. Please review.

Project coverage is 78.49%. Comparing base (1fb495e) to head (ad9a71b).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
...ect/Rock/Transforms/SortDimensionsMemoryLayout.cpp 73.03% 12 Missing and 12 partials ⚠️
mlir/lib/Conversion/TosaToRock/TosaToRock.cpp 94.70% 0 Missing and 9 partials ⚠️
mlir/lib/Conversion/TosaToRock/TosaToRockPass.cpp 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1791      +/-   ##
===========================================
- Coverage    78.60%   78.49%   -0.11%     
===========================================
  Files           99      100       +1     
  Lines        29389    29885     +496     
  Branches      4379     4451      +72     
===========================================
+ Hits         23100    23458     +358     
- Misses        4492     4591      +99     
- Partials      1797     1836      +39     
Flag Coverage Δ
mfma 78.49% <86.94%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dhernandez0 dhernandez0 mentioned this pull request Mar 28, 2025
2 tasks
// ALLOW_RETRIES: 2
// CHECK: [1 1 1]
module {
func.func private @mlir_gemm_gemm(%arg0: !migraphx.shaped<1x64x64xf32, 4096x64x1> {mhal.read_access}, %arg1: !migraphx.shaped<1x64x64xf32, 4096x64x1> {mhal.read_access}, %arg2: !migraphx.shaped<1x64x64xf32, 4096x64x1> {mhal.read_access}) -> (!migraphx.shaped<1x64x64xf32, 4096x64x1> {mhal.write_access}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Fp32 i think if you just use --clone-harness it should work without running into any accuracy issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the reason it's like this is because I copied the tests from mixr-attention.mlir. I'll change it.

@@ -0,0 +1,14 @@
// RUN: rocmlir-gen -fut mlir_gemm_gemm_where --arch %arch --clone-harness %s | rocmlir-driver -kernel-pipeline=migraphx | rocmlir-driver -host-pipeline=migraphx,highlevel | rocmlir-gen -ph -rand 1 -rand_type float -fut mlir_gemm_gemm_where_wrapper --verifier clone - | rocmlir-driver -host-pipeline mhal -kernel-pipeline full | xmir-runner --shared-libs=%linalg_test_lib_dir/libmlir_rocm_runtime%shlibext,%conv_validation_wrapper_library_dir/libconv-validation-wrappers%shlibext,%linalg_test_lib_dir/libmlir_runner_utils%shlibext,%linalg_test_lib_dir/libmlir_float16_utils%shlibext,%linalg_test_lib_dir/libmlir_c_runner_utils%shlibext,%linalg_test_lib_dir/libmlir_async_runtime%shlibext --entry-point-result=void | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it is More like gemm_where_gemm and not gemm_gemm_where

// ALLOW_RETRIES: 2
// CHECK: [1 1 1]
module {
func.func private @mlir_gemm_gemm(%arg0: !migraphx.shaped<1x4x32x32xf32, 4096x1024x32x1> {mhal.read_access},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why you need to attach mhal.read_access etc. if you are using clone-harness

// ALLOW_RETRIES: 2
// CHECK: [1 1 1]
module {
func.func private @mlir_gemm_gemm(%arg0: !migraphx.shaped<1x7x3xf32, 21x3x1> {mhal.read_access}, %arg1: !migraphx.shaped<1x3x7xf32, 21x7x1> {mhal.read_access}, %arg2: !migraphx.shaped<1x7x3xf32, 21x3x1> {mhal.read_access}) -> (!migraphx.shaped<1x7x3xf32, 21x3x1> {mhal.write_access}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you creating wrapper spearately and not using --clone-harness ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, just copied some existing attention tests and adapted to gemm+gemm. I'll change it.

@dhernandez0 dhernandez0 force-pushed the 1704-gemm-elementwise-gemm branch 2 times, most recently from 3f02a22 to d7ad776 Compare April 8, 2025 10:09
@dhernandez0 dhernandez0 force-pushed the gemm_gemm_migraphx_integration branch from 5fb22e3 to 0fe28f6 Compare April 8, 2025 10:11
@dhernandez0 dhernandez0 force-pushed the 1704-gemm-elementwise-gemm branch from 4821b84 to 8d27a5c Compare April 8, 2025 10:41
@dhernandez0 dhernandez0 force-pushed the gemm_gemm_migraphx_integration branch from 0fe28f6 to ef9080c Compare April 8, 2025 10:42
}
};

struct GemmElementwiseGemmRewritePattern
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there opportunity to use GemmGemmWrapperInterface here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I was about to push some changes re this

Comment on lines 8 to 11
func.func private @mlir_gemm_gemm(%arg0: !migraphx.shaped<1x7x3xf32, 21x3x1> {mhal.read_access},
%arg1: !migraphx.shaped<1x3x7xf32, 21x7x1> {mhal.read_access},
%arg2: !migraphx.shaped<1x7x3xf32, 21x3x1> {mhal.read_access},
%arg3: !migraphx.shaped<1x7x7xf32, 49x7x1> {mhal.read_access})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need mhal stuff here ?

Base automatically changed from 1704-gemm-elementwise-gemm to develop April 8, 2025 13:45
@dhernandez0 dhernandez0 force-pushed the gemm_gemm_migraphx_integration branch from d3372a5 to b04068b Compare April 8, 2025 14:10
@dhernandez0 dhernandez0 force-pushed the gemm_gemm_migraphx_integration branch from b04068b to ad9a71b Compare April 8, 2025 14:12
@dhernandez0 dhernandez0 merged commit 2079c51 into develop Apr 9, 2025
11 of 25 checks passed
@dhernandez0 dhernandez0 deleted the gemm_gemm_migraphx_integration branch April 9, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants