Skip to content

[SYCL] Update aspect numbering #14605

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 22, 2024
Merged

[SYCL] Update aspect numbering #14605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49ba714
update aspect numbering
aejjehint Jul 17, 2024
43a7aee
Revert aspect numbering to avoid breaking existing code
aejjehint Jul 22, 2024
dd10f48
Update aspect number in private_alloca.cpp
aejjehint Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions sycl/include/sycl/device_aspect_macros.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,70 +314,70 @@
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_limited_graph__
// __SYCL_ASPECT(ext_oneapi_limited_graph, 62)
// __SYCL_ASPECT(ext_oneapi_limited_graph, 63)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_limited_graph__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_private_alloca__
// __SYCL_ASPECT(ext_oneapi_private_alloca, 63)
// __SYCL_ASPECT(ext_oneapi_private_alloca, 64)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_private_alloca__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_cubemap__
// __SYCL_ASPECT(ext_oneapi_cubemap, 64)
// __SYCL_ASPECT(ext_oneapi_cubemap, 65)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_cubemap__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_cubemap_seamless_filtering__
// __SYCL_ASPECT(ext_oneapi_cubemap_seamless_filtering, 65)
// __SYCL_ASPECT(ext_oneapi_cubemap_seamless_filtering, 66)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_cubemap_seamless_filtering__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_1d_usm__
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d_usm, 66)
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d_usm, 67)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_1d_usm__ \
0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_1d__
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d, 67)
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d, 68)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_1d__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_2d_usm__
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d_usm, 68)
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d_usm, 69)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_2d_usm__ \
0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_2d__
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d, 69)
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d, 70)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_2d__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_3d_usm__
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d_usm, 70)
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d_usm, 71)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_3d_usm__ \
0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_3d__
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d, 71)
//__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d, 72)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_bindless_sampled_image_fetch_3d__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_queue_profiling_tag__
// __SYCL_ASPECT(ext_oneapi_queue_profiling_tag, 72)
// __SYCL_ASPECT(ext_oneapi_queue_profiling_tag, 73)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_queue_profiling_tag__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_virtual_mem__
// __SYCL_ASPECT(ext_oneapi_virtual_mem, 73)
// __SYCL_ASPECT(ext_oneapi_virtual_mem, 74)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_virtual_mem__ 0
#endif

#ifndef __SYCL_ALL_DEVICES_HAVE_ext_oneapi_cuda_cluster_group__
// __SYCL_ASPECT(ext_oneapi_cuda_cluster_group, 74)
// __SYCL_ASPECT(ext_oneapi_cuda_cluster_group, 75)
#define __SYCL_ALL_DEVICES_HAVE_ext_oneapi_cuda_cluster_group__ 0
#endif

Expand Down
26 changes: 13 additions & 13 deletions sycl/include/sycl/info/aspects.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,16 @@ __SYCL_ASPECT(ext_intel_matrix, 58)
__SYCL_ASPECT(ext_oneapi_is_composite, 59)
__SYCL_ASPECT(ext_oneapi_is_component, 60)
__SYCL_ASPECT(ext_oneapi_graph, 61)
__SYCL_ASPECT(ext_oneapi_limited_graph, 62)
__SYCL_ASPECT(ext_oneapi_private_alloca, 63)
__SYCL_ASPECT(ext_oneapi_cubemap, 64)
__SYCL_ASPECT(ext_oneapi_cubemap_seamless_filtering, 65)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d_usm, 66)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d, 67)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d_usm, 68)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d, 69)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d_usm, 70)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d, 71)
__SYCL_ASPECT(ext_oneapi_queue_profiling_tag, 72)
__SYCL_ASPECT(ext_oneapi_virtual_mem, 73)
__SYCL_ASPECT(ext_oneapi_cuda_cluster_group, 74)
__SYCL_ASPECT(ext_oneapi_limited_graph, 63)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a gap in the numbering here? It seems we're going from 61 directly to 63.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@steffenlarsen explained to me that we cannot change aspect numbers to avoid breaking existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Existing code inside the SYCL runtime or user code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When task-sequence gets re-submitted we can put it back where it was in that gap.

Copy link
Contributor

Choose a reason for hiding this comment

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

The gap is fine, but aspect values should not change. Imagine a kernel that uses double. The compiler will be told that aspect::fp64 is N and stores that with the kernel so the runtime can then check if a kernel is supported on a device. Now, let's say we change fp64 to be N+1. If the user does not recompile their kernel, the runtime will be told that the kernel requires aspect N which is no longer fp64.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be allowed in/around an ABI-breaking window? If we allow ABI-break, such an application needs to be re-compiled anyway, right?

I don't want to hold up this specific PR on this issue (in particular if we plan to re-insert the task-sequence into the gap later), so I'm fine with merging. But for the future I think we should have some process/policy defined for this kind of scenario, as it's similar to an ABI-break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be allowed in/around an ABI-breaking window? If we allow ABI-break, such an application needs to be re-compiled anyway, right?

Yes, it should, but it was not approved as an ABI/API break, so I would feel better about reverting it.

I don't want to hold up this specific PR on this issue (in particular if we plan to re-insert the task-sequence into the gap later), so I'm fine with merging. But for the future I think we should have some process/policy defined for this kind of scenario, as it's similar to an ABI-break.

Like changing an enum that crosses library boundaries, I think this would fall under our ABI/API break policy. They are sadly hard to track and sycl::aspect is even worse. Maybe we could have some tests for it. 🤔

__SYCL_ASPECT(ext_oneapi_private_alloca, 64)
__SYCL_ASPECT(ext_oneapi_cubemap, 65)
__SYCL_ASPECT(ext_oneapi_cubemap_seamless_filtering, 66)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d_usm, 67)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_1d, 68)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d_usm, 69)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_2d, 70)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d_usm, 71)
__SYCL_ASPECT(ext_oneapi_bindless_sampled_image_fetch_3d, 72)
__SYCL_ASPECT(ext_oneapi_queue_profiling_tag, 73)
__SYCL_ASPECT(ext_oneapi_virtual_mem, 74)
__SYCL_ASPECT(ext_oneapi_cuda_cluster_group, 75)
2 changes: 1 addition & 1 deletion sycl/test/check_device_code/optional_kernel_features/private_alloca.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Kernel1;
// CHECK-SAME: !sycl_used_aspects ![[#USED_ASPECTS:]]

// CHECK: ![[#USED_ASPECTS]] = !{![[#ASPECT:]]}
// CHECK: ![[#ASPECT]] = !{!"ext_oneapi_private_alloca", i32 63}
// CHECK: ![[#ASPECT]] = !{!"ext_oneapi_private_alloca", i32 64}

constexpr static sycl::specialization_id<int> size(10);

Expand Down
Loading