-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add float8 datatype to XPU OPs #1393
base: main
Are you sure you want to change the base?
Conversation
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.
kernels related: _local_scalar_dense foreach_tensor_copy cat_xpu where_xpu
@yucai-intel : what this comment tries to say? Can you, please, update PR description highlighting which XPU ops will be available for FP8 datatype after merging this PR? and whether there are any ops which won't be available with FP8 and we should expect over PRs to bring support for them?
}); | ||
|
||
// AT_DISPATCH_ALL_TYPES_AND_COMPLEX_AND3( |
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.
what's the reason to have this commented out? Either remove or add in-source comment explaining why this might be needed.
#define AT_DISPATCH_SOURCE_TYPES(TYPE, NAME, ...) \ | ||
AT_DISPATCH_SWITCH( \ | ||
TYPE, \ | ||
NAME, \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Byte, \ | ||
src_t, \ | ||
__VA_ARGS__) AT_PRIVATE_CASE_TYPE_USING_HINT(at::ScalarType::Char, src_t, __VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Long, src_t, __VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Short, src_t, __VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Int, src_t, __VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Double, src_t, __VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Float, src_t, __VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::ComplexDouble, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::ComplexFloat, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Half, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::BFloat16, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType::Bool, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType:: \ | ||
Float8_e4m3fn, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType:: \ | ||
Float8_e4m3fnuz, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType:: \ | ||
Float8_e5m2, \ | ||
src_t, \ | ||
__VA_ARGS__) \ | ||
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | ||
at::ScalarType:: \ | ||
Float8_e5m2fnuz, \ | ||
src_t, \ | ||
__VA_ARGS__)) | ||
|
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.
clang-format is a good tool, but that's not right to sacrifice human readability for the tool which can't work in certain cases. And it's known that clang-format sometimes incorrectly formats some cases. Ours being one of them.
I think in this particular sense it makes sense to disable clang-format over this code block. This will render better readable code which is always an ultimate goal of using a tool such as clang-format.
#define AT_DISPATCH_SOURCE_TYPES(TYPE, NAME, ...) \ | |
AT_DISPATCH_SWITCH( \ | |
TYPE, \ | |
NAME, \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Byte, \ | |
src_t, \ | |
__VA_ARGS__) AT_PRIVATE_CASE_TYPE_USING_HINT(at::ScalarType::Char, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Long, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Short, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Int, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Double, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Float, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::ComplexDouble, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::ComplexFloat, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Half, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::BFloat16, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Bool, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType:: \ | |
Float8_e4m3fn, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType:: \ | |
Float8_e4m3fnuz, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType:: \ | |
Float8_e5m2, \ | |
src_t, \ | |
__VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType:: \ | |
Float8_e5m2fnuz, \ | |
src_t, \ | |
__VA_ARGS__)) | |
// clang-format off | |
#define AT_DISPATCH_SOURCE_TYPES(TYPE, NAME, ...) \ | |
AT_DISPATCH_SWITCH( \ | |
TYPE, \ | |
NAME, \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Byte, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Char, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Long, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Short, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Int, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Double, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Float, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::ComplexDouble, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::ComplexFloat, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Half, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::BFloat16, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Bool, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Float8_e4m3fn, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Float8_e4m3fnuz, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Float8_e5m2, src_t, __VA_ARGS__) \ | |
AT_PRIVATE_CASE_TYPE_USING_HINT( \ | |
at::ScalarType::Float8_e5m2fnuz, src_t, __VA_ARGS__)) | |
// clang-format on |
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.
Hi, @dvrogozh , thank you for your detailed suggestions.
One point worth noting is that although clang-format's processing may not seem very readable, it is reasonable because it clearly prompts readers that these parallel functions are parameters rather than complete sentences. Maybe this is a bit confusing for you, who is an experienced developer, but for ordinary developers, such prompts are logical.
In addition, a kind reminder is that if clang-format has some inappropriate places, you can propose improvements to the overall code format review method instead of modifying individual cases. Because a single and personalized modification will cause trouble to other collaborators and reviewers, resulting in unnecessary duplication of work. After all, clear rules are more efficient than constraints.
Thank you again for your suggestions.
kernels related:
_local_scalar_dense
foreach_tensor_copy
cat_xpu
where_xpu