-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix remove rocblas tests #3100
Fix remove rocblas tests #3100
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3100 +/- ##
========================================
Coverage 91.82% 91.82%
========================================
Files 486 486
Lines 18991 18991
========================================
Hits 17438 17438
Misses 1553 1553 ☔ View full report in Codecov by Sentry. |
This build is OK for merge ✅ |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
src/targets/gpu/target.cpp
Outdated
// mlir doesn't support fp8 dot | ||
unsupported_fp8_ops.insert("dot"); | ||
unsupported_fp8_ops.insert("quant_dot"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary.
AMDMIGraphX/src/targets/gpu/fuse_mlir.cpp
Line 234 in 7011bda
return false; |
MIGraphX doesn't pick rocMLIR for the FP8 dot/quant_dots. It would use rocBLAS for fp8 dots,quant_dots.
Do you have a case where rocBLAS is disabled in MIGraphX ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is required when rocBLAS is not used (see MIGRAPHX_USE_ROCBLAS=OFF
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is added for case where rocBLAS is disabled in MIGraphX. There is variable MIGRAPHX_USE_ROCBLAS in develop branch, so when it is set to OFF we need to add unsupported fp8 ops for dot and quant dot.
src/targets/gpu/target.cpp
Outdated
#else | ||
// mlir doesn't support fp8 dot | ||
unsupported_fp8_ops.insert("dot"); | ||
unsupported_fp8_ops.insert("quant_dot"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just duplicating code for both branches. It would be better to have rocblas_fp8_available
function that just always returns false when rocblas is disabled.
c57faf5
to
eb15631
Compare
@apwojcik