Skip to content
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][Docs] Add legacy SYCL 1.2.1 image aspect #9217

Merged
merged 5 commits into from
May 5, 2023

Conversation

steffenlarsen
Copy link
Contributor

Since SYCL 2020 no longer has the SYCL 1.2.1 images, neither the device::info::image_support query nor aspect::image query guarantees that a given device support the SYCL 1.2.1 images. To alleviate this, this commit adds an extension with a new aspect ext_intel_legacy_image intended for querying support for SYCL 1.2.1 images.

To help users correctly transition to using this new aspect, a warning is issued when the user calls either platform::has or device::has with aspect::image, warning the user that SYCL 2020 images are not supported and suggesting they use the new aspect instead. In a future update, these calls will be changed to return false due to missing SYCL 2020 image support.

Since SYCL 2020 no longer has the SYCL 1.2.1 images, neither the
`device::info::image_support` query nor `aspect::image` query guarantees
that a given device support the SYCL 1.2.1 images. To alleviate this,
this commit adds an extension with a new aspect `ext_intel_legacy_image`
intended for querying support for SYCL 1.2.1 images.

To help users correctly transition to using this new aspect, a warning
is issued when the user calls either `platform::has` or `device::has`
with `aspect::image`, warning the user that SYCL 2020 images are not
supported and suggesting they use the new aspect instead. In a future
update, these calls will be changed to return false due to missing
SYCL 2020 image support.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen
Copy link
Contributor Author

Tag @intel/sycl-language-enabling-triage for awareness.

@steffenlarsen steffenlarsen temporarily deployed to aws April 26, 2023 12:28 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws April 26, 2023 14:00 — with GitHub Actions Inactive
|===


=== More sections at your discretion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a better title for this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. It has been changed!

#if !defined(SYCL_DISABLE_IMAGE_ASPECT_WARNING) && __has_attribute(diagnose_if)
#define __SYCL_WARN_IMAGE_ASPECT(aspect_param) \
__attribute__((diagnose_if( \
aspect_param == aspect::image, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here if the value of aspect_param is not known at compile time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly it would not produce the warning. For example a call like

sycl::aspect A = sycl::aspect::image;
Dev.has(A);

would not produce the warning. However, the expectation is that using the aspect directly is the most common pattern and as such should catch the majority of cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's OK. I was worried that the compiler might diagnose an error if the expression aspect_param == aspect::image cannot be evaluated at compile time.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension spec LGTM.

@steffenlarsen steffenlarsen temporarily deployed to aws April 26, 2023 20:49 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws April 27, 2023 01:54 — with GitHub Actions Inactive
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen temporarily deployed to aws April 28, 2023 09:56 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws April 28, 2023 15:10 — with GitHub Actions Inactive
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@steffenlarsen steffenlarsen temporarily deployed to aws April 28, 2023 16:01 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen requested a review from a team as a code owner May 4, 2023 10:15
@steffenlarsen
Copy link
Contributor Author

@intel/dpcpp-esimd-reviewers - In the original pass over the affected tests I missed the ESIMD-related ones. This has been amended in the newest merge with SYCL, but it should all just be requiring the new legacy image aspect instead of aspect::image.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc changes LGTM

@steffenlarsen steffenlarsen temporarily deployed to aws May 4, 2023 11:52 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws May 4, 2023 14:20 — with GitHub Actions Inactive
@jbrodman jbrodman self-requested a review May 5, 2023 11:14
@steffenlarsen steffenlarsen merged commit 78b709d into intel:sycl May 5, 2023
AlexeySachkov pushed a commit that referenced this pull request Apr 15, 2024
As mentioned in the CTS pull request
KhronosGroup/SYCL-CTS#870, two device info
descriptors, namely `info::device::image_max_array_size` and
`info::device::opencl_c_version`, have been deprecated and no longer
exist in the SYCL 2020 specification. But they still exist in our
implementation, so I'm setting up deprecation warnings for them. Also a
related test case has been revised to remove the usage of
`info::device::opencl_c_version`.

Also `info::device::usm_system_allocator` has been replaced and marked
as deprecated 2+ years ago in PR #4007
and #9217 respectively. So I'm
cleaning up a few relevant code pieces in our code as well in this PR.

---------

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants