-
Notifications
You must be signed in to change notification settings - Fork 89
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
Remove rocmlir unsupported reduce types #3766
base: develop
Are you sure you want to change the base?
Conversation
type_t::fp8e5m2fnuz_type, | ||
type_t::fp8e4m3fn_type, | ||
type_t::fp8e5m2_type}; | ||
type_t::half_type}; |
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.
These would require tests in test/verify/
section
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3766 +/- ##
========================================
Coverage 92.24% 92.24%
========================================
Files 519 519
Lines 22170 22170
========================================
Hits 20450 20450
Misses 1720 1720 ☔ View full report in Codecov by Sentry. |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
By the way, reduce f16 should not be enabled for navi. As it does not support atomic_add with packed_f16, so it would be slow. Leaving this comment here for whoever is assigned the PR. @causten |
What's the status on this PR? Is there additional work needed or is the removal of the allowed types the only thing that needs review? |
I thought this was all that needed to be done, so I created the PR. However, we need some tests and make sure reduce f16 is not enabled for Navi (except for gfx12). I think @causten will assign somebody from migraphx team to finish this. |
We are going to enable f16 for reduce in this rocmlir PR: ROCm/rocMLIR#1722
However, the code here assumes bf16 and fp8 are supported and that's not the case.