[SYCL] Throw an exception for unsupported aspect #6989

KornevNikita · 2022-10-07T15:05:10Z

The runtime must throw an exception whenever the application attempts to submit
a kernel to a device where the kernel uses a feature that is not compatible
with the device.
Co-authored-by: @AlexeySachkov

The runtime must throw an exception whenever the application attempts to submit a kernel to a device where the kernel uses a feature that is not compatible with the device.

KornevNikita · 2022-10-07T15:07:14Z

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

sycl/include/sycl/info/aspects.def

sycl/include/sycl/aspects.hpp

aelovikov-intel · 2022-10-07T19:01:42Z

sycl/include/sycl/info/aspects.def

+  __SYCL_ASPECT(usm_shared_allocations, 15, "usm_shared_allocations")
+  __SYCL_ASPECT(usm_restricted_shared_allocations, 16, "usm_restricted_shared_allocations")
+  __SYCL_ASPECT(usm_system_allocations, 17, "usm_system_allocations")
+  __SYCL_SPECIAL_ASPECT(usm_system_allocator __SYCL2020_DEPRECATED("use usm_system_allocations instead"), usm_system_allocations, "usm_system_allocations")


Can we have or a more precise definition of "special"? Also, I'd probably move deprecation message into a separate macro argument and call it

__SYCL_ASPECT_DEPRECATED(ASPECT, ID, MESSAGE)

Added the comment to describe this macro, does it look better now?

Added __SYCL_ASPECT_DEPRECATED macro & outlined these aspects into the separate file. 410f00e

aelovikov-intel · 2022-10-07T19:05:46Z

sycl/source/detail/program_manager/program_manager.cpp

+#define __SYCL_ASPECT(A, B, C)                                                 \
+  case aspect::A:                                                              \
+    return C;
+#define __SYCL_SPECIAL_ASPECT(A, B, C)


Why the deprecated alias isn't allowed here?

It's not allowed because it's "special", not deprecated. Since this only aspect doesn't have own token it will duplicate existing case and lead to compile fail.

sycl/source/detail/program_manager/program_manager.cpp

aelovikov-intel · 2022-10-07T19:07:59Z

sycl/source/detail/program_manager/program_manager.cpp

+      continue;
+    ByteArray Aspects = DeviceBinaryProperty(*It).asByteArray();
+    // 8 because we need to skip 64-bits of size of the byte array
+    auto *AIt = reinterpret_cast<const std::uint32_t *>(&Aspects[8]);


Why isn't this illegal type-punning?

I see the same case in https://github.com/intel/llvm/blob/sycl/sycl/source/detail/program_manager/program_manager.cpp#L1241. Don't really get what you mean, could you please explain?

https://stackoverflow.com/questions/63422076/why-is-type-punning-considered-ub

I think we can do this because originally these aspects were uint32_t. Maybe @steffenlarsen can add some clarity?

In that case, I don't like how the interface is defined (i.e. we shouldn't be doing this magic here), but that would be outside this PR's scope.

I have an old patch trying to amend the UB: #5537. It is a little stale now though.

I have updated #5537 and added changes for this.

I've uploaded an alternative approach in #7023.

…nsupported-feature

KornevNikita · 2022-10-10T13:47:10Z

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

sycl/include/sycl/aspects.hpp

aelovikov-intel · 2022-10-10T15:32:21Z

sycl/source/detail/program_manager/program_manager.cpp

+    auto KName = std::string_view((*It)->Name);
+    if (KName != "aspects")
+      continue;


nit: Not used to the std::string_view best practices yet, but I'd try to write like this:

Suggested change

auto KName = std::string_view((*It)->Name);

if (KName != "aspects")

continue;

if (It->Name != "aspects"sv)

continue;

I'm also not quite sure about It->Name vs (*It)->Name, but I vaguely recall operator-> behavior would enable that. Might be wrong though.

aelovikov-intel · 2022-10-10T16:36:41Z

sycl/include/sycl/info/aspects_deprecated.def

+__SYCL_ASPECT_DEPRECATED(int64_base_atomics, 7, "use atomic64 instead")
+__SYCL_ASPECT_DEPRECATED(int64_extended_atomics, 8, "use atomic64 instead")
+// Special macro for aspects that don't have own token
+__SYCL_ASPECT_DEPRECATED_ALIAS(usm_system_allocator, usm_system_allocations, "use usm_system_allocations instead")


nit: I'd personally avoid having two files, but I don't have strong argumentation.

@steffenlarsen , I'm not sure if you're "thumbing up" my "I'd avoid" or "don't have strong argumentation".

I'm approving the PR to unblock @KornevNikita but if you'd prefer to have a single file as well we can ask him to address that :) Both pre-/post-commit would work for me.

Both, really. Having a single file would be a little nicer, but I don't think it's worth sweating over. 😄

aelovikov-intel · 2022-10-10T16:38:24Z

sycl/source/detail/program_manager/program_manager.cpp

 #undef __SYCL_ASPECT_DEPRECATED
 #undef __SYCL_ASPECT

  for (RTDeviceBinaryImage::PropertyRange::ConstIterator It : ARange) {
    auto KName = std::string_view((*It)->Name);
-    if (KName != "aspects")
+    if (KName != std::string_view("aspects"))


nit: If done like this, this change is unnecessary.

Can't compile with "aspects"sv

What I mean is that my original suggestion was to eliminate named KName by using string_view on the string literal. If you keep string_view on the LHS, there is no need to transform the string literal.

But, since (*It)->Name is char*, can't we just drop KName like:
(*It)->Name != "aspects"?

That wouldn't be the same.

Okay, 51d9eab

pvchupin · 2022-10-11T18:08:01Z

@KornevNikita, please follow up on post-commit issues https://github.com/intel/llvm/actions/runs/3223149961

Related compiler changes: intel/llvm#6989 and intel/llvm#7419

…llvm-test-suite#1314) Related compiler changes: intel#6989 and intel#7419

KornevNikita added 2 commits October 7, 2022 07:57

[SYCL] Throw an exception for unsupported aspect

2b6b51c

The runtime must throw an exception whenever the application attempts to submit a kernel to a device where the kernel uses a feature that is not compatible with the device.

Add the newline

7823d65

KornevNikita requested a review from a team as a code owner October 7, 2022 15:05

KornevNikita requested review from aelovikov-intel, steffenlarsen, AlexeySachkov and dm-vodopyanov October 7, 2022 15:05

KornevNikita mentioned this pull request Oct 7, 2022

[SYCL] Test exception throwing for unsupported kernel feature intel/llvm-test-suite#1314

Merged

aelovikov-intel reviewed Oct 7, 2022

View reviewed changes

KornevNikita added 2 commits October 10, 2022 05:29

Apply suggestions

410f00e

Merge branch 'sycl' of https://github.com/intel/llvm into exception-u…

5556854

…nsupported-feature

aelovikov-intel reviewed Oct 10, 2022

View reviewed changes

Apply suggestions

3002a43

aelovikov-intel reviewed Oct 10, 2022

View reviewed changes

Apply suggestion

51d9eab

aelovikov-intel approved these changes Oct 10, 2022

View reviewed changes

pvchupin merged commit b1619f7 into intel:sycl Oct 11, 2022

KornevNikita deleted the exception-unsupported-feature branch October 14, 2022 09:15

AlexeySachkov pushed a commit to intel/llvm-test-suite that referenced this pull request Nov 21, 2022

[SYCL] Test exception throwing for unsupported kernel feature (#1314)

bde7457

Related compiler changes: intel/llvm#6989 and intel/llvm#7419

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

[SYCL] Test exception throwing for unsupported kernel feature (intel/…

204ea3e

…llvm-test-suite#1314) Related compiler changes: intel#6989 and intel#7419

[SYCL] Throw an exception for unsupported aspect #6989

[SYCL] Throw an exception for unsupported aspect #6989

Uh oh!

Conversation

KornevNikita commented Oct 7, 2022

Uh oh!

KornevNikita commented Oct 7, 2022

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

KornevNikita commented Oct 10, 2022

Uh oh!

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!

KornevNikita Oct 10, 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!

Choose a reason for hiding this comment

Uh oh!

pvchupin commented Oct 11, 2022

Uh oh!

Uh oh!

KornevNikita Oct 10, 2022 •

edited

Loading