Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Aug 5, 2021

Currently this aspect is mapped on a device info which is
being proved by get_platform_name function, claiming, that any
Intel GPU device supports BFloat16 extension. Once we have
a lower-level spec we can re-map it on the appropriate OpenCL
extension.

Spec:
#4237

Signed-off-by: Dmitry Sidorov [email protected]

@MrSidims MrSidims requested a review from AlexeySotkin August 5, 2021 10:38
@MrSidims MrSidims force-pushed the private/MrSidims/Bf16 branch 2 times, most recently from f20179c to 26c7363 Compare August 9, 2021 11:54
@MrSidims MrSidims marked this pull request as ready for review August 9, 2021 11:57
@MrSidims MrSidims requested a review from a team as a code owner August 9, 2021 11:57
@MrSidims MrSidims requested a review from s-kanaev August 9, 2021 11:57
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the changes look good to me, @romanovvlad do we allow for new features in Runtime nowadays?

Comment on lines +500 to +502
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any spec for the extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is #4237

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we wait and implement the proper solution?

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coping my answer from below.

It seems #4237 is a spec for SYCL extension. Is there any spec for "a low-level extension"?

Nothing to share right now

Could you please clarify why the temporary solution thru checking platform name is needed? Can we wait and implement the proper solution?

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")
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romanovvlad do we allow for new features in Runtime nowadays?

Yes, we do.

@@ -154,6 +154,7 @@ enum class device : cl_device_info {
atomic64 = PI_DEVICE_INFO_ATOMIC_64,
atomic_memory_order_capabilities =
PI_DEVICE_INFO_ATOMIC_MEMORY_ORDER_CAPABILITIES,
ext_intel_bf16_conversion,
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Comment on lines +500 to +502
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

@MrSidims
Copy link
Contributor Author

It seems #4237 is a spec for SYCL extension. Is there any spec for "a low-level extension"?

Nothing to share right now

Could you please clarify why the temporary solution thru checking platform name is needed? Can we wait and implement the proper solution?

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.

Currently this aspect is mapped on a device info which is
being proved by get_platform_name function, claiming, that any
Intel GPU device supports BFloat16 extension. Once we have
a lower-level spec we can re-map it on the appropriate OpenCL
extension.

Spec:
intel#4237

Signed-off-by: Dmitry Sidorov <[email protected]>
@MrSidims MrSidims force-pushed the private/MrSidims/Bf16 branch from 26c7363 to 423ff17 Compare August 12, 2021 10:15
@MrSidims
Copy link
Contributor Author

Updated minor version and ABI test

@MrSidims
Copy link
Contributor Author

E2E test: intel/llvm-test-suite#390

@MrSidims
Copy link
Contributor Author

MrSidims commented Aug 15, 2021

Closing: there are several issues with not mapping an aspect directly on a low-level extension. Also to use aspects for this device code feature we also need https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/DeviceIf/device_if.asciidoc which is not implemented. Instead I'll introduce a driver option to enable generation the BF16 conversion instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants