Skip to content
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] Implement native_specialization_constant() #8613

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

KornevNikita
Copy link
Contributor

native_specialization_constant() returns true only in JIT mode on opencl & level-zero backends (because only SPIR-V supports them)

native_specialization_constant() returns true only in JIT mode
on opencl & level-zero backends (because only SPIR-V supports them)
@KornevNikita KornevNikita requested a review from a team as a code owner March 10, 2023 15:21
@KornevNikita KornevNikita requested a review from againull March 10, 2023 15:21
@KornevNikita
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1651

@KornevNikita KornevNikita temporarily deployed to aws March 10, 2023 15:48 — with GitHub Actions Inactive
@KornevNikita KornevNikita temporarily deployed to aws March 10, 2023 16:16 — with GitHub Actions Inactive
@againull
Copy link
Contributor

Infrastructure problem on OpenCL backend.

@againull againull merged commit 4a41230 into intel:sycl Mar 10, 2023
@bader
Copy link
Contributor

bader commented Mar 11, 2023

Infrastructure problem on OpenCL backend.

I would prefer the team to fix the infrastructure problem and pass all tests "on OpenCL backend" before merging. This patch potentially can introduce a regression, which pre-commit didn't catch due to "Infrastructure problem".

@againull
Copy link
Contributor

Infrastructure problem on OpenCL backend.

I would prefer the team to fix the infrastructure problem and pass all tests "on OpenCL backend" before merging. This patch potentially can introduce a regression, which pre-commit didn't catch due to "Infrastructure problem".

Are we supposed to report this kind of issues to Tools team or do we have to fix them ourselves?

@bader
Copy link
Contributor

bader commented Mar 11, 2023

AFAIK, Tools team responsible for Jenkins system only.

@KornevNikita KornevNikita temporarily deployed to aws March 11, 2023 17:23 — with GitHub Actions Inactive
@KornevNikita KornevNikita temporarily deployed to aws March 11, 2023 23:30 — with GitHub Actions Inactive
Comment on lines +123 to +125
return !IsAOTBinary(MBinImage->getRawData().DeviceTargetSpec) &&
(MContext.get_backend() == backend::opencl ||
MContext.get_backend() == backend::ext_oneapi_level_zero);
Copy link
Contributor

Choose a reason for hiding this comment

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

Those checks should be re-ordered: we should do less expensive backend checks first and only then do more expensive string comparison checks.

Comment on lines +116 to +120
return (
(strcmp(Format, __SYCL_PI_DEVICE_BINARY_TARGET_SPIRV64_X86_64) ==
0) ||
(strcmp(Format, __SYCL_PI_DEVICE_BINARY_TARGET_SPIRV64_GEN) == 0) ||
(strcmp(Format, __SYCL_PI_DEVICE_BINARY_TARGET_SPIRV64_FPGA) == 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead do something like Format != JIT SPIR TARGET? I understand that it might be more tricky, because that target string is probably a substring of AOT targets, but it should allow us to do less string comparisons.

@KornevNikita
Copy link
Contributor Author

@AlexeySachkov okay, I'll make a follow-up patch

@KornevNikita
Copy link
Contributor Author

#8630

steffenlarsen pushed a commit that referenced this pull request Mar 15, 2023
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.

4 participants