-
Notifications
You must be signed in to change notification settings - Fork 796
[SYCL][NFC] Move kernel launch property parsing out of sycl::handler
#19589
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
base: sycl
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors kernel launch property parsing to move it out of the sycl::handler
class into a separate helper structure for better code organization and reusability. The motivation is to enable reuse of kernel launch property parsing code in no-handler submission paths.
Key Changes:
- Creates
KernelLaunchPropertyWrapper
struct to encapsulate property parsing logic - Converts kernel launch properties to use
std::optional
to track if properties have been set - Updates all references to kernel properties to use the new structure with dereferencing
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
sycl/include/sycl/detail/kernel_launch_helper.hpp | Implements new KernelLaunchPropertyWrapper with property parsing methods |
sycl/include/sycl/handler.hpp | Updates handler to use new property wrapper and adds helper methods |
sycl/source/detail/handler_impl.hpp | Makes handler_impl inherit from KernelLaunchPropertyWrapper |
sycl/source/handler.cpp | Updates property handling to use new structure and adds property setter method |
sycl/unittests/scheduler/*.cpp | Updates mock handlers to dereference optional kernel properties |
sycl/test/virtual-functions/properties-negative.cpp | Updates expected error location from handler.hpp to kernel_launch_helper.hpp |
sycl/test/extensions/properties/non_esimd_kernel_fp_control.cpp | Updates expected error location |
sycl/test/include_deps/sycl_detail_core.hpp.cpp | Adjusts include dependency ordering |
sycl/unittests/program_manager/arg_mask/EliminatedArgMask.cpp | Updates mock handler to use dereferenced properties |
MKernelClusterSize.fill(0); | ||
} | ||
|
||
KernelLaunchPropertiesT &operator=(const KernelLaunchPropertiesT &Other) { |
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.
The custom assignment operator should have a corresponding custom copy constructor, destructor, and move operations following the Rule of Five, or use the default implementations if possible.
Copilot uses AI. Check for mistakes.
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.
Yeah, why not = default;
?
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.
In hindsight, overriding the assignment operator wasn't the best choice as what I instead wanted is to have a method for taking union of two properties. This method is required as on some code paths we parse multiple property sets for the same kernel. So, I've instead added another method takeUnionOfProperties
for the same.
typename KernelType> | ||
static void parseProperties([[maybe_unused]] PropertyProcessor h, | ||
[[maybe_unused]] const KernelType &KernelFunc) { | ||
// Changing values in this will break ABI/API. |
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.
Needs a test under sycl/test/abi/**
then.
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.
I just moved the code for StableKernelCacheConfig
enum out from sycl::handler. Nevertheless, I think the comment here was misleading: StableKernelCacheConfig
enum is modeled after ur_kernel_cache_config_t
(https://oneapi-src.github.io/unified-runtime/core/api.html?highlight=ur_kernel_cache_config_t#ur-kernel-cache-config-t) that why changing StableKernelCacheConfig
would break compatibility between UR and SYCL.
Under the preview breaking flag, I've completely removed the usage of this enum and used ur_kernel_cache_config_t
directly.
// In some code paths, kernel launch properties are set multiple times | ||
// for the same kernel, i.e why using std::optional to avoid overriding | ||
// previously set properties. |
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.
// In some code paths, kernel launch properties are set multiple times | |
// for the same kernel, i.e why using std::optional to avoid overriding | |
// previously set properties. | |
// In some code paths, kernel launch properties are set multiple times | |
// for the same kernel, that is why using std::optional to avoid overriding | |
// previously set properties. |
However, why not fix multiple parsing instead?
MKernelClusterSize.fill(0); | ||
} | ||
|
||
KernelLaunchPropertiesT &operator=(const KernelLaunchPropertiesT &Other) { |
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.
Yeah, why not = default;
?
template < | ||
bool IsESIMDKernel, typename PropertyProcessorPtrT, | ||
typename PropertiesT = ext::oneapi::experimental::empty_properties_t> | ||
static KernelLaunchPropertiesT processProperties(PropertiesT Props, |
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.
I was thinking about inlining launch properties processing before, but it makes even less sense to have them separate now.
sycl/include/sycl/handler.hpp
Outdated
if (auto prop = | ||
detail::KernelLaunchPropertyWrapper::parseProperties<NameT>( | ||
KernelFunc, this)) { | ||
setKernelLaunchProperties(*prop); | ||
} |
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.
optional
part needs a dedicated comment (near parseProperties
, I'd think). Is that a performance optimization?
@@ -3483,6 +3516,7 @@ class __SYCL_EXPORT handler { | |||
bool IsDeviceImageScoped, size_t NumBytes, | |||
size_t Offset); | |||
|
|||
#ifndef __INTEL_PREVIEW_BREAKING_CHANGES |
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.
Please make sure to have a local e2e run with split build/run mode where build is done with the sycl-toolchain
built before this PR and run is using libsycl.so with this PR's changes.
Motivation is to decouple kernel launch property parsing from
sycl::handler
so that we can reuse this code for no-handler submission path.