[SYCL] Add aspect for bfloat16 by AidanBeltonS · Pull Request #5720 · intel/llvm

AidanBeltonS · 2022-03-03T12:00:50Z

This PR adds a new aspect ext_oneapi_bfloat16 to allow a runtime check for if the device supports the bfloat16 floating point type.
Only the CUDA implementation for checking if the device supports this aspect is added.

Updated test: intel/llvm-test-suite#888

steffenlarsen · 2022-03-03T12:26:57Z

I do not see the bf16 aspect in the SYCL 2020 specification. If this is an extension it should be named accordingly and may need documentation.

MrSidims · 2022-03-03T12:34:19Z

I do not see the bf16 aspect in the SYCL 2020 specification. If this is an extension it should be named accordingly and may need documentation.

There is actually an extension, but bf16 aspect there is known under different name. @AidanBeltonS can these names be united?

AidanBeltonS · 2022-03-03T14:26:09Z

I do not see the bf16 aspect in the SYCL 2020 specification. If this is an extension it should be named accordingly and may need documentation.

There is actually an extension, but bf16 aspect there is known under different name. @AidanBeltonS can these names be united?

Yes, ideally we would unite the two aspects.
Currently the ext_intel_bf16_conversion aspect is not backend agnostic.
Unifying the two would be good as it would generalize the existing aspect so that it can be used by other backends.

I think this has been discussed a bit before: #5645 (comment)

s-kanaev

Other than the comments, the change LGTM.

s-kanaev · 2022-03-04T11:15:48Z

sycl/include/CL/sycl/aspects.hpp

  ext_oneapi_native_assert = 31,
  host_debuggable = 32,
  ext_intel_gpu_hw_threads_per_eu = 33,
+  bf16 = 34


To reduce changes in future:

Suggested change

bf16 = 34

bf16 = 34,

s-kanaev · 2022-03-04T11:27:20Z

sycl/test/abi/sycl_symbols_linux.dump

 _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE65809EEENS3_12param_traitsIS4_XT_EE11return_typeEv
 _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE65810EEENS3_12param_traitsIS4_XT_EE11return_typeEv
 _ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE69632EEENS3_12param_traitsIS4_XT_EE11return_typeEv
+_ZNK2cl4sycl6device8get_infoILNS0_4info6deviceE73728EEENS3_12param_traitsIS4_XT_EE11return_typeEv


Probably, a PATCH version in CMakeLists should be incremented to denote a non-breaking change. @alexbatashev , please, correct me if I'm wrong.
Please, refer to https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ABIPolicyGuide.md#abi-versioning-policy.

I am not sure if the PATCH version should be updated as in the abi-versioning-policy docs it states. MAJOR, MINOR, and PATCH versions are not updated between releases.

gmlueck · 2022-03-09T13:21:57Z

sycl/doc/extensions/experimental/SYCL_EXT_ONEAPI_BF16_CONVERSION.asciidoc

@@ -1,4 +1,4 @@
-= SYCL_INTEL_bf16_conversion
+= SYCL_ONEAPI_bfloat16


I think your branch must be out-of-date because these specification file names are now all lower case. You should merge the base and resolve conflicts (or maybe it will just be easier to start a new PR).

The name of the file should stay in sync with the feature-test macro. Since you are renaming the feature-test macro to "SYCL_EXT_ONEAPI_BFLOAT16_CONVERSION", the file name should be "sycl_ext_oneapi_bfloat16_conversion.asciidoc". The name of the extension (on line 1 of this file) should also be lowercase: "sycl_ext_oneapi_bfloat16_conversion".

However, maybe you are preparing this extension to encompass all bfloat16 features, rather than just conversion? As I mentioned in #5645, I think it would be better to have a single bloat16 extension like this. In that case, the name of the feature-test macro should be "SYCL_EXT_ONEAPI_BFLOAT16", the name of the extension should be "sycl_ext_oneapi_bfloat16", and the name of the file should be "sycl_ext_oneapi_bfloat16.asciidoc".

Finally, the header file "include/CL/sycl/feature_test.hpp" should also be updated to define the new feature-test macro, and you should remove the definition of the old feature-test macro.

No changes to the extension are in this PR anymore. The extension was updated by #5393.
So the new aspect is inline with the the extension

Thanks. I think this looks good.

@smaslov-intel, do you want a chance to review this from the implementation point of view?

smaslov-intel · 2022-05-16T22:59:50Z

sycl/include/CL/sycl/detail/pi.h

  PI_DEVICE_INFO_ATOMIC_MEMORY_SCOPE_CAPABILITIES = 0x11000,
  PI_DEVICE_INFO_GPU_HW_THREADS_PER_EU = 0x10112,
  PI_DEVICE_INFO_BACKEND_VERSION = 0x10113,
+  PI_EXT_ONEAPI_DEVICE_INFO_BFLOAT16 = 0x12000,


Let's not create more than necessary gaps in numbering. Please use 0x1FFFF

Okay thanks. I have updated the value

sycl/include/CL/sycl/detail/pi.h

smaslov-intel · 2022-05-17T01:01:39Z

Why don't you add a test for real "bfloat16" support, if the aspect is reported?

AidanBeltonS · 2022-05-17T10:22:12Z

Why don't you add a test for real "bfloat16" support, if the aspect is reported?

Could you clarify what you mean by this? Finding out if the device supports bfloat16 is backend dependent and involves calling native CUDA functions.
I have also added a test in the llvm-test-suite that checks if the aspect can be used.

smaslov-intel · 2022-05-17T15:40:26Z

I have also added a test in the llvm-test-suite that checks if the aspect can be used.

Can you also add a test that really uses bfloat16 after the aspect check returns true?

AidanBeltonS · 2022-05-24T14:46:19Z

Can you also add a test that really uses bfloat16 after the aspect check returns true?

Yes, I have made a change to BFloat16/bfloat16_type_cuda.cpp to run the test after the aspect returns true.

…ct_bf16

AidanBeltonS · 2022-05-24T15:14:25Z

/verify with intel/llvm-test-suite#888

pvchupin · 2022-06-11T03:34:35Z

@AidanBeltonS, can you fix conflicts please?

AidanBeltonS · 2022-06-13T09:21:28Z

@AidanBeltonS, can you fix conflicts please?

I have resolved all conflicts.

This PR adds the bf16 aspect to aspects.cpp test. Depends on: intel/llvm#5720

intel#5720 added a new specialization of get_info for info::device::ext_oneapi_bfloat16 but no Windows symbol was added to the dump file. This commit adds the missing symbol. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

#5720 added a new specialization of get_info for info::device::ext_oneapi_bfloat16 but no Windows symbol was added to the dump file. This commit adds the missing symbol. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

This PR adds the bf16 aspect to aspects.cpp test. Depends on: intel#5720

AidanBeltonS requested review from a team as code owners March 3, 2022 12:00

AidanBeltonS requested a review from s-kanaev March 3, 2022 12:00

AidanBeltonS mentioned this pull request Mar 3, 2022

[SYCL] Add bf16 aspect intel/llvm-test-suite#888

Merged

MrSidims self-requested a review March 3, 2022 12:31

AidanBeltonS requested a review from AlexeySotkin March 3, 2022 14:26

JackAKirk mentioned this pull request Mar 3, 2022

[SYCL][Doc] math functions added to bfloat16 ext #5645

Merged

s-kanaev reviewed Mar 4, 2022

View reviewed changes

AidanBeltonS requested a review from a team as a code owner March 9, 2022 09:57

gmlueck reviewed Mar 9, 2022

View reviewed changes

t4c1 mentioned this pull request Mar 15, 2022

[SYCL] Add tests for bf16 builtins operating on storage types intel/llvm-test-suite#897

Open

AidanBeltonS changed the title ~~[SYCL] Add aspect for bf16~~ [SYCL] Add aspect for bfloat16 Mar 16, 2022

JackAKirk mentioned this pull request Mar 16, 2022

[SYCL][CUDA] bfloat16 in oneapi namespace and also supporting CUDA #5393

Merged

aidan.belton added 5 commits May 16, 2022 10:11

add aspect for bf16

b476add

make aspect oneapi extension

25a6707

Update bf16_conversion extension

e055600

Undo update bf16_conversion extension

992ba74

fix rebase

1ee0543

AidanBeltonS force-pushed the aspect_bf16 branch from 634dc94 to 1ee0543 Compare May 16, 2022 12:46

Fix werror

abeca3f

smaslov-intel reviewed May 16, 2022

View reviewed changes

sycl/include/CL/sycl/detail/pi.h Outdated Show resolved Hide resolved

Add device info comment and update value

cf60751

Update symbols dump

70fcad2

Merge branch 'sycl' of https://github.com/AidanBeltonS/llvm into aspe…

1f1c8c8

…ct_bf16

pvchupin requested review from smaslov-intel and removed request for AlexeySotkin June 8, 2022 00:00

smaslov-intel previously approved these changes Jun 8, 2022

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into aspect_bf16

3f6b592

AidanBeltonS dismissed smaslov-intel’s stale review via 3f6b592 June 13, 2022 08:28

Fix PI_INVALID_VALUE error

43e26d9

pvchupin merged commit f84fc32 into intel:sycl Jun 13, 2022

pvchupin pushed a commit to intel/llvm-test-suite that referenced this pull request Jun 13, 2022

[SYCL] Add bf16 aspect (#888)

d22b329

This PR adds the bf16 aspect to aspects.cpp test. Depends on: intel/llvm#5720

steffenlarsen mentioned this pull request Jun 14, 2022

[SYCL] Adds missing get_info Windows symbol #6305

Merged

JackAKirk mentioned this pull request Jun 28, 2022

[SYCL][CUDA] Test cases for bfloat16 math/elem wise joint_matrix intel/llvm-test-suite#975

Merged

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

[SYCL] Add bf16 aspect (intel/llvm-test-suite#888)

abcb5bf

This PR adds the bf16 aspect to aspects.cpp test. Depends on: intel#5720

		@@ -1,4 +1,4 @@
		= SYCL_INTEL_bf16_conversion
		= SYCL_ONEAPI_bfloat16

Conversation

AidanBeltonS commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffenlarsen commented Mar 3, 2022

Uh oh!

MrSidims commented Mar 3, 2022

Uh oh!

AidanBeltonS commented Mar 3, 2022

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaslov-intel May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smaslov-intel commented May 17, 2022

Uh oh!

AidanBeltonS commented May 17, 2022

Uh oh!

smaslov-intel commented May 17, 2022

Uh oh!

AidanBeltonS commented May 24, 2022

Uh oh!

AidanBeltonS commented May 24, 2022

Uh oh!

pvchupin commented Jun 11, 2022

Uh oh!

AidanBeltonS commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AidanBeltonS commented Mar 3, 2022 •

edited

Loading

smaslov-intel May 16, 2022 •

edited

Loading