-
Notifications
You must be signed in to change notification settings - Fork 803
[DOC] Add SYCL_INTEL_bf16_conversion to sycl/doc/extensions/README.md #4428
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
Merged
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should we mention that the aspect for this is not implemented yet?
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 prevents us from implementing the aspect?
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 need a low-level runtime specification to map this aspect on. Also, aspects aren't helping now for device-code features without device_if feature.
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 part isn't true. An application can use logic on the host (
device::has(aspect)) to decide which kernel to submit to a device. A typical pattern would be to define two versions of a kernel (e.g. by defining the kernel as a template) and submit one or the other depending on whether the device supports an aspect.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.
But it doesn't help for a device compiler to decide whether it should compile the kernel. If the device compiler doesn't know the appropriate instruction it will emit an error (best case) or crash (worst case) even if a user put kernel invocation in
if (0) { /*...*/ }condition.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 have a design that will fix this (OptionalDeviceFeatures), and that design requires each optional feature (like this one) to have an aspect.
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.
I understand that we may not be able to implement all parts of this API at present. However, the missing aspect support is technically a bug in our implementation now. The extension spec says that it is available, but our implementation doesn't provide it. Do we have an open bug report to track this?
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.
Yeap, that's why I've said, that aspects aren't helping now and we need device_if feature :)
So, taking in account, that we don't have a low-level runtime spec to map the aspect on (and taking a (almost) random number for now will cause an ABI breakage when we will re-map the aspect on the extension), and since without
device_iffeature there is no value in the aspect for this feature - it wasn't added.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.
Just for reference: #4266 - this PR was adding the appropriate aspect, but it was dismissed and closed for the reasons I described above.
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.
Internally - yes.