Skip to content

Conversation

@uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Oct 28, 2025

This PR refactors kernel property parsing in the SYCL implementation by consolidating property parsing logic from the handler into a centralized helper structure. The refactoring moves property validation and processing from handler.hpp into kernel_launch_helper.hpp, so that the same logic can be re-used on the no-handler path as well.

Copy link
Contributor

Copilot AI left a 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 pull request refactors kernel property parsing in the SYCL implementation by consolidating property parsing logic from the handler into a centralized helper structure. The refactoring moves property validation and processing from handler.hpp into kernel_launch_helper.hpp, introducing a new PropsHolder structure for marshalling kernel properties and simplifying the handler interface.

  • Introduces a new kernel_launch_properties_v1 namespace with PropsHolder and MarshalledProperty templates for centralizing property parsing
  • Moves property validation logic from handler::processProperties to detail::processKernelProperties
  • Adds handler::setKernelLaunchProperties method that delegates property processing to kernel_data.hpp

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
sycl/include/sycl/detail/kernel_launch_helper.hpp Introduces centralized property parsing infrastructure with PropsHolder and processKernelProperties
sycl/include/sycl/handler.hpp Refactors handler to use new property parsing infrastructure and removes inline property processing
sycl/source/detail/kernel_data.hpp Adds property validation and setter methods for kernel launch properties
sycl/source/handler.cpp Implements setKernelLaunchProperties delegation and guards deprecated methods
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 from handler.hpp to kernel_launch_helper.hpp
sycl/test/include_deps/*.cpp Updates include dependency ordering to reflect new header relationships
sycl/test/abi/sycl_symbols_*.dump Adds new ABI symbols for setKernelLaunchProperties
sycl/include/sycl/ext/oneapi/experimental/cluster_group_prop.hpp Makes get_cluster_size() const-qualified

parseProperties([[maybe_unused]] const KernelType &KernelFunc) {

KernelPropertyHolderStructTy props;
#ifndef __SYCL_DEVICE_ONLY__
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why do we need this guard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This guard was stale - earlier we used to call into libsycl from parseProperties function. But, now that I've split parsing and setting/validation of properties it's no longer required. I've moved this guard to setKernelLauchProperties method instead.

Comment on lines 212 to 214
void validateAndSetKernelLaunchProperties(
const detail::KernelPropertyHolderStructTy &Kprop, bool HasGraph,
const device_impl &dev) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we still need this? Can we pass Kprop as-is down to the UR calls?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants