-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Add BFloat16 aspect #4266
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -493,6 +493,23 @@ template <> struct get_device_info<device, info::device::parent_device> { | |
} | ||
}; | ||
|
||
// Specialization for bfloat16 | ||
template <> | ||
struct get_device_info<bool, info::device::ext_intel_bf16_conversion> { | ||
static bool get(RT::PiDevice dev, const plugin &Plugin) { | ||
// TODO: We claim, that all Intel GPU devices support bfloat16 conversion | ||
// feature but we'd better have a low-level extension to query for support | ||
// of the feature | ||
Comment on lines
+500
to
+502
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any spec for the extension? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it is #4237 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems #4237 is a spec for SYCL extension. Is there any spec for "a low-level extension"? Could you please clarify why the temporary solution thru checking platform name is needed? Can we wait and implement the proper solution? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd like to have it merged soon. Is it ok if we merge it as is and update the implementation later, when we have the proper solution? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coping my answer from below.
Nothing to share right now
So users could already write bfloat16 - sensitive code using aspect traits deciding for them which version of kernel should be enqueued: with or without bf16 conversion feature. |
||
platform plt = | ||
get_device_info<platform, info::device::platform>::get(dev, Plugin); | ||
std::string platform_name = plt.get_info<info::platform::name>(); | ||
if (platform_name == "Intel(R) OpenCL HD Graphics") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe, there should be a better way to verify the device is either of Intel's GPUs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now there is no low-level runtime specification for the feature, so it's the best, what came in my mind. Do you have any ideas? |
||
return true; | ||
|
||
return false; | ||
} | ||
}; | ||
|
||
// SYCL host device information | ||
|
||
// Default template is disabled, all possible instantiations are | ||
|
@@ -998,6 +1015,11 @@ inline bool get_device_info_host<info::device::ext_intel_mem_channel>() { | |
return false; | ||
} | ||
|
||
template <> | ||
inline bool get_device_info_host<info::device::ext_intel_bf16_conversion>() { | ||
return false; | ||
} | ||
|
||
cl_uint get_native_vector_width(size_t idx); | ||
|
||
// USM | ||
|
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.
Are we going to map it to a specific value in future?
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