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] Add handling for wrapped sampler #1942

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

Fznamznon
Copy link
Contributor

This adds proper initialization for sampler object that is wrapped by a
struct. This is temporary change because it contradicts the OpenCL and
SPIR-V spec, since the struct with sampler opaque type field appears as
kernel argument. We need it now to fix crashes in OpenCL backends.

This adds proper initialization for sampler object that is wrapped by a
struct. This is temporary change because it contradicts the OpenCL and
SPIR-V spec, since the struct with sampler opaque type field appears as
kernel argument. We need it now to fix crashes in OpenCL backends.
@elizabethandrews
Copy link
Contributor

Should a Sema test be added as well?

@Fznamznon
Copy link
Contributor Author

Should a Sema test be added as well?

Not sure that this AST test has value, first because handling of wrapped sampler that I'm adding is fully identical to wrapped accessor handling (and we have a test for this case). Second, this solution is temporary and intended to fix crashes in OpenCL backends caused by missing raw sampler argument, that I'm checking in CodeGen test.
To make proper solution we need at least full decomposing of structs that is being worked here #1877 . So, such AST test will be almost fully rewritten in the near time.

@elizabethandrews
Copy link
Contributor

Frontend changes LGTM. I am not familiar with runtime tests so I defer the review of test to someone more familiar.

Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

The added runtime test case LGTM

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

I am okay with this temporary change to address crashes.

@bader bader merged commit b7a34be into intel:sycl Jun 23, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jun 24, 2020
* upstream/sycl:
  [SYCL] Implement braced-init-list or a number as range for queue::parallel_for (intel#1931)
  [SYCL][Doc] Add SYCL_INTEL_accessor_properties extension specification (intel#1925)
  [SYCL-PTX] Add builtins for the relational category (intel#1831)
  [SYCL][CUDA] Remove unnecessary memfence (intel#1935)
  [SYCL] Add handling for wrapped sampler (intel#1942)
  [SYCL] Release notes for June'20 DPCPP implementation update (intel#1948)
  [SYCL] Fix assert when calling get_binaries() on host (intel#1944)
  [SYCL] Fix check for reqd_sub_group_size attribute mismatches (intel#1905)
@keryell
Copy link
Contributor

keryell commented Jun 29, 2020

If you have some ideas on how to solve this in the general OpenCL/SPIR-V case, participate to the Khronos OpenCL tooling meetings.
I have the feeling we should replace all these opaque types with something which is more C/C++ friendly.

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.

6 participants