[SYCL][NFCI][ABI-Break] Move handler members to impl #14460

steffenlarsen · 2024-07-05T12:52:31Z

This moves some of the members in the handler class to its impl class. Doing so allows developers to change these arguments without breaking ABI. This also moves more implementation details, such as command-group classes, launch configuration information, argument information and HostTask tracking, into sources to avoid hard-to-find ABI breaks in the communication between headers and runtime library.

In addition to this, the following improvements are made:

The HostKernel class has been simplified to no longer have call and runOnHost functions.
The HostTask wrapper class has been moved to sources and the owner has been changed from a unique_ptr to a shared_ptr, which prevents the need for including host_task_impl.hpp in odd places.

This moves some of the members in the handler class to its impl class. Doing so allows developers to change these arguments without breaking ABI. This also moves more implementation details, such as command-group classes, launch configuration information, argument information and HostTask tracking, into sources to avoid hard-to-find ABI breaks in the communication between headers and runtime library. In addition to this, the following improvements are made: * The HostKernel class has been simplified to no longer have call and runOnHost functions. * The HostTask wrapper class has been moved to sources and the owner has been changed from a unique_ptr to a shared_ptr, which prevents the need for including host_task_impl.hpp in odd places. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

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

bader · 2024-07-05T18:21:33Z

sycl/source/detail/scheduler/graph_builder.cpp

@@ -1329,15 +1329,15 @@ Command *Scheduler::GraphBuilder::connectDepEvent(
  ExecCGCommand *ConnectCmd = nullptr;

  try {
-    std::unique_ptr<detail::HostTask> HT(new detail::HostTask);
+    std::shared_ptr<detail::HostTask> HT(new detail::HostTask);


Is this change NFC?

Functionally, we co not change the intended behavior of the pointer. It does change the way it could be used in the future, though I do not see how that would be a problem. The benefit for the runtime is that the headers can have it be an incomplete type.

…lems

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

…lems

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

…lems

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

…lems

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

sycl/test-e2e/Basic/sycl_2020_images/host_task_sampled_image_read_nearest.cpp

aelovikov-intel · 2024-07-08T17:47:33Z

Looks good to me, waiting for it to be moved out of draft state.

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

The group class cannot be used in host code, but one of its ctors uses asserts that only work on host. These assertions use % and / on ranges and MSVC thinks that these could potentially cause division by 0. Since these are unreachable they can be safely removed. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2024-07-11T18:06:21Z

sycl/include/sycl/group.hpp

-                  "global range is not multiple of local");
-    __SYCL_ASSERT((((G / L) - GroupRange).size() == 0) &&
-                  "inconsistent group constructor arguments");
-  }


For some reason, these changes makes MSVC think that this could potentially cause division by 0. However, this ctor cannot be called on host (since group is not usable on host) so these asserts carry no meaning.

steffenlarsen · 2024-07-16T05:40:12Z

@intel/unified-runtime-reviewers - Friendly ping.

steffenlarsen · 2024-07-16T14:30:46Z

@intel/sycl-graphs-reviewers - It looks like @EwanC is off today. Could one of you please have a look so we can get this in before the ABI-break window closes? 😄

fabiomestre

Graph related changes LGTM

This moves some of the members in the handler class to its impl class. Doing so allows developers to change these arguments without breaking ABI. This also moves more implementation details, such as command-group classes, launch configuration information, argument information and HostTask tracking, into sources to avoid hard-to-find ABI breaks in the communication between headers and runtime library. In addition to this, the following improvements are made: * The HostKernel class has been simplified to no longer have call and runOnHost functions. * The HostTask wrapper class has been moved to sources and the owner has been changed from a unique_ptr to a shared_ptr, which prevents the need for including host_task_impl.hpp in odd places. --------- Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

hdelan · 2024-07-17T11:03:59Z

It looks like clang format issues were introduced in this PR and not caught by CI

hdelan · 2024-07-17T11:06:16Z

sycl/source/handler.cpp

    CommandGroup.reset(new detail::CGPrefetchUSM(MDstPtr, MLength,
-                                                 std::move(CGData), MCodeLoc));
+                                                 std::move(impl->CGData), MCodeLoc));


Here and other places are wider than 80 cols. I'm not sure how this wasn't caught by CI

intel#14460 appears to have introduced clang formatting violations, despite pre-commit not reporting these. This commit makes the formatting changes that should have been part of that patch. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2024-07-17T11:16:41Z

It looks like clang format issues were introduced in this PR and not caught by CI

Thank you for notifying me about this, @hdelan! It is a very good question why this was not caught by the clang-formatter. Either way, I have opened a patch to fix the formatting missed: #14597

…4589) * [14460](#14460) moved handler members to impl and basically fixed exclusions we had in this test. * Also match "struct" as well, not only "class", these resulted in several new hits but those doesn't seem to cross ABI boundary, because they are compile-time properties fully processed in the headers.

#14460 appears to have introduced clang formatting violations, despite pre-commit not reporting these. This commit makes the formatting changes that should have been part of that patch. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

The changes in intel#14460 removed the seemingly unused functions for running kernels on on host. However, this turned out to be used by debuggers as they need the kernel code to be in the host executable. This commit adds a simplified version of the kernel instantiation that the aforementioned patch removed. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

The changes in #14460 removed the seemingly unused functions for running kernels on on host. However, this turned out to be used by debuggers as they need the kernel code to be in the host executable. This commit adds a simplified version of the kernel instantiation that the aforementioned patch removed. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen requested a review from aelovikov-intel July 5, 2024 12:52

steffenlarsen had a problem deploying to WindowsCILock July 5, 2024 12:53 — with GitHub Actions Failure

steffenlarsen mentioned this pull request Jul 5, 2024

[SYCL] [ABI-Break] Partial implementation of sycl_ext_oneapi_cuda_cluster_group #14113

Merged

Fix shallowing integer

5805367

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

bader reviewed Jul 5, 2024

View reviewed changes

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

8878682

…lems

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 05:47 — with GitHub Actions Failure

steffenlarsen added 2 commits July 7, 2024 23:02

Fix failure

e339faf

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

Move a few more members

d93ef7b

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

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 06:16 — with GitHub Actions Failure

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 06:37 — with GitHub Actions Error

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

4fa05e8

…lems

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 06:56 — with GitHub Actions Failure

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 07:18 — with GitHub Actions Failure

steffenlarsen added 3 commits July 8, 2024 02:33

Fix AddLifetimeSharedPtrStorage arg

69abfe6

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

Fix padding

d1ca544

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

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

a2f49a5

…lems

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 10:02 — with GitHub Actions Failure

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 10:34 — with GitHub Actions Failure

Pad NDRDescT global size in NumWorkGroups case

5f99c91

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

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 13:19 — with GitHub Actions Failure

steffenlarsen added 2 commits July 8, 2024 09:33

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

8fdfcdd

…lems

Fix build issue

ad2e20c

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

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 16:44 — with GitHub Actions Failure

aelovikov-intel reviewed Jul 8, 2024

View reviewed changes

sycl/test-e2e/Basic/sycl_2020_images/host_task_sampled_image_read_nearest.cpp Outdated Show resolved Hide resolved

steffenlarsen had a problem deploying to WindowsCILock July 8, 2024 17:58 — with GitHub Actions Failure

Fix formatting and missed padding case

c389dc0

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

steffenlarsen had a problem deploying to WindowsCILock July 9, 2024 06:37 — with GitHub Actions Failure

steffenlarsen had a problem deploying to WindowsCILock July 11, 2024 08:44 — with GitHub Actions Failure

steffenlarsen temporarily deployed to WindowsCILock July 11, 2024 09:03 — with GitHub Actions Inactive

Add Windows symbols and change namings to match other members

abb11cc

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

aelovikov-intel approved these changes Jul 11, 2024

View reviewed changes

steffenlarsen commented Jul 11, 2024

View reviewed changes

steffenlarsen temporarily deployed to WindowsCILock July 11, 2024 18:08 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to WindowsCILock July 11, 2024 19:46 — with GitHub Actions Inactive

Merge branch 'sycl' into steffen/move_handler_elems

affd0ba

steffenlarsen temporarily deployed to WindowsCILock July 12, 2024 06:03 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to WindowsCILock July 12, 2024 06:37 — with GitHub Actions Inactive

aarongreig approved these changes Jul 16, 2024

View reviewed changes

steffenlarsen requested a review from EwanC July 16, 2024 12:41

fabiomestre approved these changes Jul 16, 2024

View reviewed changes

aelovikov-intel merged commit f7ffa52 into intel:sycl Jul 16, 2024
14 checks passed

againull mentioned this pull request Jul 16, 2024

[SYCL][Test] Update ABI test after moving handler members to impl #14589

Merged

ianayl added a commit to ianayl/sycl that referenced this pull request Jul 17, 2024

Amended change from intel#14460

70075d3

hdelan reviewed Jul 17, 2024

View reviewed changes

steffenlarsen mentioned this pull request Jul 17, 2024

[SYCL][NFC] Fix formatting after handler member changes #14597

Merged

steffenlarsen mentioned this pull request Sep 2, 2024

[SYCL] Add host kernel instantiation for debuggers #15256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][NFCI][ABI-Break] Move handler members to impl #14460

[SYCL][NFCI][ABI-Break] Move handler members to impl #14460

steffenlarsen commented Jul 5, 2024

bader Jul 5, 2024

steffenlarsen Jul 8, 2024

aelovikov-intel commented Jul 8, 2024

steffenlarsen Jul 11, 2024

steffenlarsen commented Jul 16, 2024

steffenlarsen commented Jul 16, 2024

fabiomestre left a comment

hdelan commented Jul 17, 2024

hdelan Jul 17, 2024

steffenlarsen commented Jul 17, 2024

[SYCL][NFCI][ABI-Break] Move handler members to impl #14460

[SYCL][NFCI][ABI-Break] Move handler members to impl #14460

Conversation

steffenlarsen commented Jul 5, 2024

bader Jul 5, 2024

Choose a reason for hiding this comment

steffenlarsen Jul 8, 2024

Choose a reason for hiding this comment

aelovikov-intel commented Jul 8, 2024

steffenlarsen Jul 11, 2024

Choose a reason for hiding this comment

steffenlarsen commented Jul 16, 2024

steffenlarsen commented Jul 16, 2024

fabiomestre left a comment

Choose a reason for hiding this comment

hdelan commented Jul 17, 2024

hdelan Jul 17, 2024

Choose a reason for hiding this comment

steffenlarsen commented Jul 17, 2024