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.
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
[SYCL] Move bfloat support from experimental to supported. #6524
[SYCL] Move bfloat support from experimental to supported. #6524
Changes from 8 commits
6014cef
bdd88e5
73ed541
0fe1884
feb9d5f
129f53f
2115f09
3c2eb80
74aa175
bd05711
f8e894c
2ad68f6
4b78c03
0fce16d
4bcb383
35308f8
fa045e2
3322d6a
b12fd94
87b0f09
f217eb4
a908b11
aab4c78
a2568ba
4d7a22b
38e5ad4
b9accad
ca7880a
dc3b2b5
c955d36
1aa6ad3
ff04ce1
802f502
8d7f46a
190f2a3
84c50f3
df058ba
fed4d1d
28259d0
c11115b
6b05a2a
a82d73a
3fc8885
1ec6838
105094b
432e775
c135643
4eca414
8876ac8
f0f2727
17673bf
1094b8c
8d40228
c5a85cf
cf8f6e0
5e50646
45d3e70
a7be718
cac1c18
208c09a
46f406d
6830857
46e5278
10fc9a3
6195545
437e34a
09dc4c5
386353e
0f93586
48f3cac
d33cb10
28992c2
ec28c8b
b958fc7
ec70b20
1b86012
3e1e681
8c633d3
1a59e03
b2fd6cc
fab2e54
35b8910
a05c872
ac5f603
6d45ed1
077d0fe
2ff6a9d
d7c80ee
20d13df
cd1d0a2
4bf60b9
45c32f7
5de1bf7
6ec2bb9
49e9cd1
2065060
e24e57b
41098ab
37b05f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Some of the new text you added has very long lines. Please respect the 80-column limit as documented in the template.
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.
OK, done.
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 like this conversion to
sycl::half
. However, we should also add the opposite conversion fromsycl::half
tobfloat16
:Do we also need conversion to / from
double
?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 PR is intended to move the current bfloat16 support out of experimental space. Any changes to the level of bfloat16 support can be done in future PRs.
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.
On Intel platforms the bfloat16 to/from float is done using the __spirv_ConvertBF16ToFINTELoperator. I suspect a double version of that does not exist.
Float to double conversion can be made in the usual C++ way more efficiently in hardware. A direct version of bfloat16 to double conversion in software will involve more bit twiddling than the float conversion where only trailing 0 bits of fraction need to be inserted.
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 sycl::half class includes conversions to/from float. Those kick in when bfloat16 is used with sycl::half, so conversions between bfloat16 and sycl::half are not needed.
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 you saying that we should remove this conversion from
bfloat16
tosycl::half
?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, its not needed.
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 item was revisited and it turns out that sycl::half <-> bfloat16 conversions are needed. They have been 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.
Sorry for joining the discussion late. May be it's a nitpick, but should we tell, that conversion half <-> bfloat16 follows IEEE 754 float <-> half conversion? In other words, what happens, if bfloat16 value overflows half range? Also are we adding last 3 fraction bits stochastically or they are guarantied to be zero (or it's implementation detail)?