Skip to content

[SYCL] Add sycl-jit e2e feature #19724

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

Open
wants to merge 6 commits into
base: sycl
Choose a base branch
from

Conversation

KornevNikita
Copy link
Contributor

The project may be built with --disable-jit.
Updating the tests that fail without this feature.

The project may be built with --disable-jit
@KornevNikita KornevNikita requested a review from a team as a code owner August 6, 2025 12:24
@KornevNikita KornevNikita requested a review from againull August 6, 2025 12:24
@@ -970,6 +970,9 @@ def get_sycl_ls_verbose(sycl_device, env):
config.available_features.add("device-config-file")
config.substitutions.append(("%device_config_file_include_flag", ""))

if config.enable_sycl_jit == "ON":
Copy link
Contributor Author

@KornevNikita KornevNikita Aug 6, 2025

Choose a reason for hiding this comment

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

Looks like in other cases (e.g config.allow_unknown_arch) we just check the value of variable like:
if not config.allow_unknown_arch:
Anybody knows - why it doesn't work here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if relevant: #18893

@KornevNikita
Copy link
Contributor Author

Doesn't work as expected, moving to draft for now.

@KornevNikita KornevNikita marked this pull request as draft August 7, 2025 12:27
@KornevNikita
Copy link
Contributor Author

KornevNikita commented Aug 8, 2025

@intel/dpcpp-devops-reviewers this pre-commit looks really weird, like something was broken. I believe it's impossible for my patch, right?
UPD. Ah, okay, I see. Sorry to bother you

@KornevNikita KornevNikita changed the title Add sycl-jit e2e feature [SYCL] Add sycl-jit e2e feature Aug 8, 2025
@KornevNikita KornevNikita marked this pull request as ready for review August 8, 2025 14:33
@KornevNikita KornevNikita requested a review from againull August 8, 2025 14:34
@KornevNikita
Copy link
Contributor Author

@againull could you please take one more look?

againull
againull previously approved these changes Aug 8, 2025
Comment on lines +977 to +979
elif platform.system() == "Windows":
if os.path.exists(os.path.join(config.sycl_libs_dir, "sycl-jit.lib")):
config.available_features.add("sycl-jit")
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: sycl-jit.lib is required at compile time, but at runtime, sycl-jit.dll must be available in the PATH. However, since the .lib and .dll are typically distributed together, I believe checking for the .lib should be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's probably better to check only for dll in this case.
Currently in implementation sycl-jit.dll and sycl-jit.so are loaded at runtime via LoadLibrary* and dlopen.
I.e. if I am not mistaken test can be compiled even if those files don;'t exist. And only in runtime we need .so and .dll.

@@ -47,6 +47,7 @@ class E2EExpr(BooleanExpression):
"pdtracker",
"ze_debug",
"device-config-file",
"sycl-jit",
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not mistaken we don't need this requirement at build-time, i.e. this is "run-mode" only feature and needs to be removed from this list. Please see my second comment.

@againull againull dismissed their stale review August 8, 2025 21:32

Some comments need to be addressed.

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