[SYCL] Remove sycl_ext_oneapi_default_context in preview-breaking mode#22769
[SYCL] Remove sycl_ext_oneapi_default_context in preview-breaking mode#22769softwaresat wants to merge 3 commits into
Conversation
Robertkq
left a comment
Robertkq
left a comment
There was a problem hiding this comment.
LGTM, good work!
Only note: In this repository, the body of the PR usually becomes the commit message of the Squash & Merge procedure when the PR is eventually merged, as such I think you should try to keep the body of the PR as concise as possible on the changes made and to add comments to the PR for explaining the decisions behind them
| /// Return this platform's default context | ||
| /// | ||
| /// \return the default context | ||
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES |
There was a problem hiding this comment.
So we don't forget to remove the comment as well.
| /// Return this platform's default context | |
| /// | |
| /// \return the default context | |
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES | |
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES | |
| /// Return this platform's default context | |
| /// | |
| /// \return the default context |
| This extension has been removed and is no longer supported in {dpcpp}. | ||
| Applications should use the `sycl_khr_default_context` extension instead. |
There was a problem hiding this comment.
Usually we use this wording for removed extensions from the template:
| This extension has been removed and is no longer supported in {dpcpp}. | |
| Applications should use the `sycl_khr_default_context` extension instead. | |
| This extension is no longer implemented in DPC++. This specification is being archived only for historical purposes. The APIs defined in this specification no longer exist and cannot be used. |
Although the API is still implemented actually and will be removed prior to 27.0 (8.0.0). I don't mind moving the spec to removed right now so we don't forget to move it later. @intel/dpcpp-doc-reviewers could you please comment on this?
UPD. see the comment from Greg. I've updated the issue tasks - #22581
There was a problem hiding this comment.
I think it would be better to leave the specification in the "deprecated" directory until it is removed in the default case (i.e. even when -fpreview-breaking-changes is not passed).
Do you think there is a danger that we will forget to do this? Would it help us to remember if we put a comment in the Asciidoc file like:
// __INTEL_PREVIEW_BREAKING_CHANGES: Move this specification to the "removed"
// folder when the feature is removed at the next breaking changes window.
That way we will see the comment when we search for __INTEL_PREVIEW_BREAKING_CHANGES
| @@ -1,2 +1,2 @@ | |||
| This extension has been deprecated, but the specification is still available | |||
There was a problem hiding this comment.
I think we just remove the extension file from supported and deprecated directories.
UPD. let's keep it for now. Probably a comment like here would be enough.
| @@ -0,0 +1,32 @@ | |||
| // RUN: %clangxx -fsycl -fpreview-breaking-changes -fsyntax-only -Xclang -verify %s | |||
There was a problem hiding this comment.
I think that might be excessive (I mean having this test). @KseniyaTikhomirova WDYT?
4 participants
What does this PR do?
This PR removes the deprecated
sycl_ext_oneapi_default_contextextension when preview breaking changes are enabled.It guards
platform::ext_oneapi_get_default_context()and theSYCL_EXT_ONEAPI_DEFAULT_CONTEXTfeature-test macro with__INTEL_PREVIEW_BREAKING_CHANGES. Normal builds continue to expose thedeprecated API for backward compatibility, while preview-breaking builds only
expose the replacement
platform::khr_get_default_context()API.The supported
device::ext_oneapi_get_default_context()API and itsSYCL_EXT_ONEAPI_DEVICE_DEFAULT_CONTEXTfeature-test macro remain available.This PR also:
deprecated/toremoved/.platform::ext_oneapi_get_default_context()is unavailable in preview mode.SYCL_EXT_ONEAPI_DEFAULT_CONTEXTis not defined in preview mode.device::ext_oneapi_get_default_context()remains available.platform::khr_get_default_context()remains available.Why was this PR needed?
sycl_ext_oneapi_default_contexthas been deprecated in favor of thesycl_khr_default_contextextension. Deprecated interfaces need to be hiddenbehind
__INTEL_PREVIEW_BREAKING_CHANGESso they can be removed during the nextABI-breaking window without immediately breaking users of normal builds.
While investigating the extension, I found that the platform and device classes
both contain methods named
ext_oneapi_get_default_context(), but they belongto different extensions. This change removes only the deprecated platform
extension and preserves the supported device extension.
What are the relevant issue numbers?
Addresses the
sycl_ext_oneapi_default_contextportion of #22581.This PR intentionally does not close #22581 because the issue tracks the removal
of multiple deprecated extensions, each of which is intended to be handled in a
separate PR.
Does this PR meet the acceptance criteria?