-
Notifications
You must be signed in to change notification settings - Fork 736
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][NFCI][ABI-Break] Move handler members to impl #14460
[SYCL][NFCI][ABI-Break] Move handler members to impl #14460
Conversation
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>
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change NFC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
sycl/test-e2e/Basic/sycl_2020_images/host_task_sampled_image_read_nearest.cpp
Outdated
Show resolved
Hide resolved
Looks good to me, waiting for it to be moved out of draft state. |
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
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>
"global range is not multiple of local"); | ||
__SYCL_ASSERT((((G / L) - GroupRange).size() == 0) && | ||
"inconsistent group constructor arguments"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@intel/unified-runtime-reviewers - Friendly ping. |
@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? 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
It looks like clang format issues were introduced in this PR and not caught by CI |
CommandGroup.reset(new detail::CGPrefetchUSM(MDstPtr, MLength, | ||
std::move(CGData), MCodeLoc)); | ||
std::move(impl->CGData), MCodeLoc)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
…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>
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: