-
Notifications
You must be signed in to change notification settings - Fork 86
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
Enable GEMM/dot for FP8 using hipblasLT #3577
Conversation
src/targets/gpu/target.cpp
Outdated
@@ -129,9 +129,11 @@ std::vector<pass> target::get_passes(migraphx::context& gctx, const compile_opti | |||
unsupported_fp8e4m3fnuz_ops.insert("argmin"); | |||
|
|||
std::set<std::string> unsupported_fp8ocp_ops = {}; | |||
// TODO update with hipBLASLt support | |||
// TODO: remove this when the flag is removed | |||
#if !MIGRAPHX_ENABLE_HIPBLASLT_GEMM |
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.
Probably need to do MIGRAPHX_DECLARE_ENV_VAR(MIGRAPHX_ENABLE_HIPBLASLT_GEMM)
at the top of the file...
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.
I'm not sure what that does exactly. Is the env variable not declared elsewhere?
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 seems to be the way MIGraphX handles env vars. Doing this provides functions like enabled/disabled etc to work with the env vars, so we can set any of the following values for the env variable:
Set to "1", "enable", "enabled", "yes", or "true" to use.
Also, I missed to mention that we should do if(not enabled(MIGRAPHX_ENABLE_HIPBLASLT_GEMM{}))
instead of !MIGRAPHX_ENABLE_HIPBLASLT_GEMM
, for guarding using the env variable.
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.
Does this environment variable not clash with the one already declared in lowering.cpp? Thanks.
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.
I don't think they should clash.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3577 +/- ##
===========================================
- Coverage 92.17% 92.17% -0.01%
===========================================
Files 513 513
Lines 21560 21558 -2
===========================================
- Hits 19873 19871 -2
Misses 1687 1687 ☔ View full report in Codecov by Sentry. |
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.
Approved. Is there any test case that verifies this set of Ops. Thanks.
Yes, there are the |
Created an issue tracking the preprocessor conditional I added: #3592 |
I tried using CMake's |
The types are defined as enum in hip not in hipblaslt: So we shouldn't check the hipblaslt version. It looks like hipblaslt already checks the hip version and then defines Which looks like it is to enable the non-fnuz types: So we can probably use |
Works well with the variable |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
Looks to work correctly on Navi4X