-
Notifications
You must be signed in to change notification settings - Fork 87
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
bit_cast operator #3655
base: develop
Are you sure you want to change the base?
bit_cast operator #3655
Conversation
CharlieL7
commented
Nov 22, 2024
- Converts between types while keeping the data the same. Basically an extension of reinterpret_cast without undefined behavior
- Needed for conversion pass between fp8e4m3fn to fp8e4m3fnuz.
typename From, | ||
MIGRAPHX_REQUIRES(not is_any_vec<To>()), | ||
MIGRAPHX_REQUIRES(is_trivially_copyable<To>{} and is_trivially_copyable<From>{})> | ||
inline constexpr auto bit_cast(From fr) noexcept |
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.
There is no need for this overload, since the vec_transform
works with non vectors as well.
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.
It doesn't work properly with the usage of bit_cast in float8_impl
.
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.
We shouldn't be vectorizing fp8 types.
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.
Also, vec_transform
already does the same check for is_any_vec
and it just calls the function directly, so I dont see how that would cause an error with fp8 types.
MIGRAPHX_REQUIRES(is_trivially_copyable<To>{} and is_trivially_copyable<From>{})> | ||
inline constexpr To bit_cast(From fr) noexcept | ||
{ |
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 function should return auto
. To
is not valid return with vector types. If you do bit_cast<int8_t>(vec<uint8_t, 2>{})
it should return vec<int8_t, 2>{}
not int8_t
. The vec_transform
functor already takes care of figuring out the return type for you, so you can just return auto
instead.
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.
Same issue. When compiling float8 for GPU this version is used.
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.
Same issue? The return type is wrong for this. If you bit_cast a vector type you should get a vector type, not the scalar 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.
The issue comes from the usage here:
f_inf = migraphx::bit_cast<float>(if_inf); |
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.
That takes a scalar input not a vector input.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3655 +/- ##
===========================================
- Coverage 92.18% 92.17% -0.01%
===========================================
Files 513 514 +1
Lines 21596 21616 +20
===========================================
+ Hits 19908 19925 +17
- Misses 1688 1691 +3 ☔ 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 |
What is the exact error are you getting when doing only one overload for template <typename To,
typename From,
MIGRAPHX_REQUIRES(is_trivially_copyable<To>{} and is_trivially_copyable<From>{})>
inline constexpr auto bit_cast(From fr) noexcept
{
return vec_transform(fr)([](auto x) -> To {
static_assert(sizeof(To) == sizeof(decltype(x)));
return __builtin_bit_cast(To, x);
});
} |