[SYCL][Docs] Update sycl_ext_intel_usm_address_spaces and fix ctors #7680

steffenlarsen · 2022-12-07T12:46:08Z

This commit updates the sycl_ext_intel_usm_address_spaces extension to adhere to SYCL 2020 multi_ptr and updates the extension specification to use the new extension template.

Additionally this commit fixes the multi_ptr constructors for the extension address spaces.

This commit updates the sycl_ext_intel_usm_address_spaces extension to adhere to SYCL 2020 multi_ptr and updates the extension specification to use the new extension template. The extension is moved from draft to experimental. Additionally this commit fixes the multi_ptr constructors for the extension address spaces. Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2022-12-07T12:47:58Z

@GarveyJoe & @gmlueck - The new extension template does not have the notion of a draft extension, so this moves sycl_ext_intel_usm_address_spaces to experimental. Any objections?

steffenlarsen · 2022-12-07T12:48:56Z

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

gmlueck · 2022-12-07T13:58:21Z

The new extension template does not have the notion of a draft extension, so this moves sycl_ext_intel_usm_address_spaces to experimental. Any objections?

The previous extension was "supported" not "draft". I don't mind making it "experimental", but I was under the impression that the FPGA team needed this to be "supported".

Do the changes in this PR break compatibility with existing code?

steffenlarsen · 2022-12-07T14:04:20Z

The previous extension was "supported" not "draft". I don't mind making it "experimental", but I was under the impression that the FPGA team needed this to be "supported".

It was under the supported folder, but the extension specification stated that the extension itself was in draft state, with roughly the same disclaimers as we use for experimental.

Do the changes in this PR break compatibility with existing code?

It should not. It just ensures it works correctly for non-legacy multi_ptr. It was not correctly tested before because to use the right address space we currently require __ENABLE_USM_ADDR_SPACE__ to be defined. intel/llvm-test-suite#1439 should ensure we continue to test this.

sycl/doc/extensions/experimental/sycl_ext_intel_usm_address_spaces.asciidoc

gmlueck · 2022-12-07T14:20:22Z

sycl/doc/extensions/experimental/sycl_ext_intel_usm_address_spaces.asciidoc

+
+template<typename ElementType, access::decorated IsDecorated = access::decorated::legacy>
+using host_ptr = multi_ptr<ElementType, access::address_space::ext_intel_global_host_space, IsDecorated>
+```


Clarify which namespace these are in. I think they are currently in sycl, but that's not the right place. They should either be in sycl::ext::intel or sycl::ext::intel::experimental, depending on the status of this extension.

I think we should deprecate the existing aliases and add new ones in the correct namespace.

Currently it is in the sycl namespace, but I agree they should be moved. Should we also add raw_ and decorated_ variants while we are in here? @GarveyJoe - Are you okay if we just move them or would you prefer we keep the old ones in the sycl namespace as deprecated for now?

Please keep the old ones working with deprecation warnings for now so that we can migrate gradually. FYI, @ajaykumarkannan

I have added a deprecation warning to the sycl::{device|host}_ptr aliases, but it seems that clang isn't handling deprecations of templated aliases correctly at the moment, so users won't be told until that is fixed.

The new pointer aliases are:

sycl::ext::intel::device_ptr

sycl::ext::intel::raw_device_ptr

sycl::ext::intel::decorated_device_ptr

sycl::ext::intel::host_ptr

sycl::ext::intel::raw_host_ptr

sycl::ext::intel::decorated_host_ptr

following the same definitions as other pointer aliases with similar naming in SYCL 2020.

gmlueck · 2022-12-07T14:22:35Z

sycl/include/sycl/access/access.hpp

@@ -356,6 +356,17 @@ template <typename ToT, typename FromT> inline ToT cast_AS(FromT from) {
      return reinterpret_cast<ToT>(from);
 #endif // defined(__NVPTX__) || defined(__AMDGCN__)
  } else
+#ifdef __ENABLE_USM_ADDR_SPACE__


Is this extension is only enabled if the user defines this macro? That isn't consistent with other extensions and it isn't documented in the extension specification.

My understanding is that if this isn't defined it will just use the same address space qualifier as for global_space, so the extension is still supported but does not have much of an effect.

gmlueck · 2022-12-07T14:27:59Z

It was under the supported folder, but the extension specification stated that the extension itself was in draft state, with roughly the same disclaimers as we use for experimental.

The language evolution team discussed this in the past and decided that the "Draft" notice in the extension specifications was not a reliable indication that the extension was unsupported. This is left over from before we had an official extension spec template, and virtually all of the old extensions said they were "Draft".

BTW, thanks for taking the time to migrate this spec to the new template. We have a number of old specifications that should be migrated, and it's good to chip away at the backlog.

Signed-off-by: Larsen, Steffen <[email protected]>

GarveyJoe · 2022-12-07T22:57:50Z

I second Greg's comments about the "draft" not being reliable. This extension is fully supported and being used by customers today. Can we keep the new version under "supported" and make sure that any functional changes are staged to cause minimal migration pain?

Signed-off-by: Larsen, Steffen <[email protected]>

…sor_ctors

steffenlarsen · 2022-12-08T10:41:34Z

I second Greg's comments about the "draft" not being reliable. This extension is fully supported and being used by customers today. Can we keep the new version under "supported" and make sure that any functional changes are staged to cause minimal migration pain?

Thank you, @GarveyJoe! It has been moved back to supported and as such I have incremented the feature-test macro to indicate the change.

gmlueck

The extension is moved from draft to experimental.

You probably want to edit your first PR comment to remove this sentence, so it doesn't appear in the final squashed commit message.

sycl/doc/extensions/supported/sycl_ext_intel_usm_address_spaces.asciidoc

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2022-12-08T14:03:51Z

You probably want to edit your first PR comment to remove this sentence, so it doesn't appear in the final squashed commit message.

I have changed the description of the PR which should be what we normally reflect in the merge commit. Changing the description of the first PR would require a force-push which we generally try to avoid as it makes github trip for reviewers.

gmlueck · 2022-12-08T14:12:45Z

I have changed the description of the PR which should be what we normally reflect in the merge commit. Changing the description of the first PR would require a force-push which we generally try to avoid as it makes github trip for reviewers.

Yes, that's what I mean. Your term "description of the PR" is what I meant by "first PR comment".

steffenlarsen · 2022-12-12T12:28:58Z

@GarveyJoe - Are you happy with the changes?

intel#7680 deprecated device_ptr and host_ptr under the sycl namespace and intended to add them under the sycl::ext::intel namespace instead, but did not. This commit adds them to the namespace as originally intended. They are already implemented in the extension headers. Signed-off-by: Larsen, Steffen <[email protected]>

#7680 deprecated device_ptr and host_ptr under the sycl namespace and intended to add them under the sycl::ext::intel namespace instead, but did not. This commit adds them to the namespace as originally intended. They are already implemented in the extension headers. Signed-off-by: Larsen, Steffen <[email protected]>

They have been deprecated in intel#7680 more than a year ago.

They have been deprecated in #7680 more than a year ago. --------- Co-authored-by: Greg Lueck <[email protected]>

steffenlarsen requested a review from GarveyJoe December 7, 2022 12:46

steffenlarsen requested review from a team as code owners December 7, 2022 12:46

steffenlarsen requested a review from dm-vodopyanov December 7, 2022 12:46

steffenlarsen mentioned this pull request Dec 7, 2022

[SYCL] Add testing for multi_ptr with extended USM address spaces intel/llvm-test-suite#1439

Merged

gmlueck reviewed Dec 7, 2022

View reviewed changes

Apply suggestions

5697fef

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen added 3 commits December 8, 2022 00:24

Move back to supported and move pointers to sycl::ext::intel namespace

f059872

Signed-off-by: Larsen, Steffen <[email protected]>

Deprecate old pointer aliases and add new ones

3a7c22e

Signed-off-by: Larsen, Steffen <[email protected]>

Merge remote-tracking branch 'intel/sycl' into steffen/fix_mptr_acces…

36c5a95

…sor_ctors

gmlueck reviewed Dec 8, 2022

View reviewed changes

sycl/doc/extensions/supported/sycl_ext_intel_usm_address_spaces.asciidoc Outdated Show resolved Hide resolved

Remove SPIR-V requirement notification

0e6458e

Signed-off-by: Larsen, Steffen <[email protected]>

gmlueck approved these changes Dec 8, 2022

View reviewed changes

dm-vodopyanov approved these changes Dec 12, 2022

View reviewed changes

GarveyJoe approved these changes Dec 12, 2022

View reviewed changes

steffenlarsen merged commit 4a9e9a0 into intel:sycl Dec 12, 2022

steffenlarsen mentioned this pull request Dec 16, 2022

[SYCL][Docs] Reintroduce device_ptr and host_ptr under namespace #7814

Merged

aelovikov-intel mentioned this pull request Apr 1, 2024

[SYCL] Remove deprecated sycl::host_ptr/sycl::device_ptr #13240

Merged

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Apr 1, 2024

[SYCL] Remove deprecated sycl::host_ptr/sycl::device_ptr

e2250a9

They have been deprecated in intel#7680 more than a year ago.

aelovikov-intel added a commit that referenced this pull request Apr 2, 2024

[SYCL] Remove deprecated sycl::host_ptr/sycl::device_ptr (#13240)

ec3d9ee

They have been deprecated in #7680 more than a year ago. --------- Co-authored-by: Greg Lueck <[email protected]>

masterleinad mentioned this pull request Apr 22, 2024

Replace deprecated sycl::device_ptr/sycl::host_ptr kokkos/kokkos#6958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Docs] Update sycl_ext_intel_usm_address_spaces and fix ctors #7680

[SYCL][Docs] Update sycl_ext_intel_usm_address_spaces and fix ctors #7680

steffenlarsen commented Dec 7, 2022 •

edited

Loading

steffenlarsen commented Dec 7, 2022

steffenlarsen commented Dec 7, 2022

gmlueck commented Dec 7, 2022

steffenlarsen commented Dec 7, 2022

gmlueck Dec 7, 2022

steffenlarsen Dec 7, 2022

GarveyJoe Dec 7, 2022

steffenlarsen Dec 8, 2022

gmlueck Dec 7, 2022

steffenlarsen Dec 7, 2022

gmlueck commented Dec 7, 2022

GarveyJoe commented Dec 7, 2022

steffenlarsen commented Dec 8, 2022

gmlueck left a comment

steffenlarsen commented Dec 8, 2022

gmlueck commented Dec 8, 2022

steffenlarsen commented Dec 12, 2022

[SYCL][Docs] Update sycl_ext_intel_usm_address_spaces and fix ctors #7680

[SYCL][Docs] Update sycl_ext_intel_usm_address_spaces and fix ctors #7680

Conversation

steffenlarsen commented Dec 7, 2022 • edited Loading

steffenlarsen commented Dec 7, 2022

steffenlarsen commented Dec 7, 2022

gmlueck commented Dec 7, 2022

steffenlarsen commented Dec 7, 2022

gmlueck Dec 7, 2022

Choose a reason for hiding this comment

steffenlarsen Dec 7, 2022

Choose a reason for hiding this comment

GarveyJoe Dec 7, 2022

Choose a reason for hiding this comment

steffenlarsen Dec 8, 2022

Choose a reason for hiding this comment

gmlueck Dec 7, 2022

Choose a reason for hiding this comment

steffenlarsen Dec 7, 2022

Choose a reason for hiding this comment

gmlueck commented Dec 7, 2022

GarveyJoe commented Dec 7, 2022

steffenlarsen commented Dec 8, 2022

gmlueck left a comment

Choose a reason for hiding this comment

steffenlarsen commented Dec 8, 2022

gmlueck commented Dec 8, 2022

steffenlarsen commented Dec 12, 2022

steffenlarsen commented Dec 7, 2022 •

edited

Loading