Skip to content

[SYCL][DOC] Refactor OCL global variable spec #5659

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

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Feb 24, 2022

We decided that the OpenCL API Specification should not contain
detailed information about how the APIs interact with SPIR-V. Instead,
we want this information in the OpenCL SPIR-V Environment
Specification.

Change this extension specification accordingly.

We decided that the OpenCL API Specification should not contain
detailed information about how the APIs interact with SPIR-V.  Instead,
we want this information in the OpenCL SPIR-V Environment
Specification.

Change this extension specification accordingly.
@gmlueck gmlueck requested a review from bashbaug February 24, 2022 19:51
@gmlueck gmlueck requested a review from a team as a code owner February 24, 2022 19:51
bashbaug
bashbaug previously approved these changes Feb 28, 2022
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. 👍

At some point we may decide that the modifications to the SPIR-V environment spec belong more with whatever extension or tooling describes support SPV_INTEL_global_variable_decorations rather than here, but we can work that out later if necessary. The important thing for now is that the name resolution will be documented in the SPIR-V environment spec.

Comment on lines 293 to 294
parameter which identifies the variable. This parameter is interpreted as
follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider saying explicitly that this section only applies for programs created via SPIR-V, perhaps by adding to the last sentence:

For a cl_program created from SPIR-V, this parameter is interpreted as follows...

Debatably this is unnecessary because this section is in the SPIR-V environment specification, but I still think it is clearer to say it explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Done in 39cad4d

Comment from Ben, clarify that these rules apply only if the module
was created from SPIR-V.
@bader
Copy link
Contributor

bader commented Mar 1, 2022

@gmlueck, I think Ben's approval is enough to merge this change. Do you agree?
We should consider making @bashbaug a code owner for sycl/doc/design/opencl-extensions/* documents.

@gmlueck
Copy link
Contributor Author

gmlueck commented Mar 1, 2022

@gmlueck, I think Ben's approval is enough to merge this change. Do you agree?
We should consider making @bashbaug a code owner for sycl/doc/design/opencl-extensions/* documents.

Yes, I agree.

It might be better to create a GitHub "team" for the OpenCL spec reviewers, similar to the way we have "intel/dpcpp-spirv-reviewers" for SPIR-V specs. We could then add @bashbaug to that team and potentially others.

@bader
Copy link
Contributor

bader commented Mar 1, 2022

It might be better to create a GitHub "team" for the OpenCL spec reviewers, similar to the way we have "intel/dpcpp-spirv-reviewers" for SPIR-V specs. We could then add @bashbaug to that team and potentially others.

@pvchupin, could you help with this, please?

@bader bader merged commit 2ceeba5 into intel:sycl Mar 1, 2022
@gmlueck gmlueck deleted the gmlueck/ocl-spirv-env branch March 1, 2022 22:29
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