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.

Sign up for GitHub

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

Jump to bottom

[SYCL][Doc] Update property_list specification to revision 2 #5338

Merged
merged 5 commits into from Jan 27, 2022
Merged

[SYCL][Doc] Update property_list specification to revision 2 #5338

merged 5 commits into from Jan 27, 2022

Conversation

rolandschulz
Copy link
Contributor

  • Distinquish between key and value
  • Rename foo_v into foo
  • Rename foo into foo_key
  • Rename property_list into properties
  • Other cleanup

@rolandschulz
[SYCL] property_list rev2
43c6c84 
- Distinquish between key and value
- Rename foo_v into foo
- Rename foo into foo_key
- Rename property_list into properties
- Other cleanup
@rolandschulz rolandschulz requested a review from a team as a code owner January 19, 2022 09:16
Pennycook
Pennycook requested changes Jan 19, 2022
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

This mostly LGTM. Thank you for making all these changes!

I found a few typos, and a few instances where I'm not sure if you're missing a _key suffix. All the ones I'm not sure of end with a "?"; please ignore them if I'm wrong.

sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
gmlueck
gmlueck reviewed Jan 19, 2022
View reviewed changes
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc Outdated Show resolved Hide resolved
@Pennycook Pennycook mentioned this pull request Jan 19, 2022
Merged
rolandschulz and others added 3 commits January 19, 2022 20:47
@rolandschulz @Pennycook @gmlueck
Apply suggestions from code review
416922c 
Co-authored-by: John Pennycook <john.pennycook@intel.com>
Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
@rolandschulz
Address comments
34eebe2
@rolandschulz
rename of file
dca95ed
Pennycook
Pennycook previously approved these changes Jan 20, 2022
View reviewed changes
@bader bader changed the title [SYCL] property_list rev2 [SYCL][Doc] Update property_list specification to revision 2 Jan 20, 2022
gmlueck
gmlueck approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

LGMT!

@steffenlarsen steffenlarsen mentioned this pull request Jan 21, 2022
Merged
gmlueck
gmlueck reviewed Jan 24, 2022
View reviewed changes
sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_properties.asciidoc

Compile-time-constant properties are an important building block for classes and functions that have a need to propagate compile-time information for semantic and optimization purposes, while runtime properties continue to serve an important role in dynamic parameter specification.

This extension introduces `sycl::ext::oneapi::properties`, which is a replacement for `sycl::property_list` that supports the storage and manipulation of compile-time-constant properties in addition to runtime properties.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought of something else ... didn't we decide that the first implementation will be experimental? If so, we should change the namespace to sycl::ext::oneapi::experimental::properties.

Tagging @steffenlarsen to see if this matches the implementation he is working on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought of something else ... didn't we decide that the first implementation will be experimental? If so, we should change the namespace to sycl::ext::oneapi::experimental::properties.

Yes, I believe we agreed that it should be experimental for now. Whether or not the namespace change should be done in this PR I'll let you decide.

@bader bader added the Documentation Missing documentation for the code, compiler or runtime features, etc. label Jan 27, 2022
steffenlarsen
steffenlarsen approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlueck gmlueck self-requested a review January 27, 2022 15:45
@gmlueck
Copy link
Contributor

gmlueck commented Jan 27, 2022

Note that I revoked my approval for now. I think we need to adjust the namespace in this document to state that these APIs are in the sycl::ext::oneapi::experimental namespace. See my unresolved comment on this.

@bader
Copy link
Contributor

bader commented Jan 27, 2022

Note that I revoked my approval for now. I think we need to adjust the namespace in this document to state that these APIs are in the sycl::ext::oneapi::experimental namespace. See my unresolved comment on this.

@gmlueck, do you want @rolandschulz to make this change in this PR or doing it in a separate PR is fine?

@gmlueck
Copy link
Contributor

gmlueck commented Jan 27, 2022

@gmlueck, do you want @rolandschulz to make this change in this PR or doing it in a separate PR is fine?

I guess I'm not opposed to doing it in a separate PR, but I'm not sure why that helps. Does @rolandschulz prefer to do it in a separate PR?

@bader
Copy link
Contributor

bader commented Jan 27, 2022

@gmlueck, do you want @rolandschulz to make this change in this PR or doing it in a separate PR is fine?

I guess I'm not opposed to doing it in a separate PR, but I'm not sure why that helps. Does @rolandschulz prefer to do it in a separate PR?

This option was mentioned by @steffenlarsen in this comment.

Note that #5338 does not move compile-time property list to the experimental namespace, but there will be another PR shortly after to move it, so the changes here should be correct.

@steffenlarsen
Copy link
Contributor

This option was mentioned by @steffenlarsen in this comment.

Note that #5338 does not move compile-time property list to the experimental namespace, but there will be another PR shortly after to move it, so the changes here should be correct.

Given approvals I assumed that would be the solution, following the previous discussion. I do not have a preference for whether or not it is included in this PR.

@gmlueck
Copy link
Contributor

gmlueck commented Jan 27, 2022

OK, I re-added my approval. I'm OK with fixing the namespace in a separate PR.

@bader bader merged commit 33fdc58 into intel:sycl Jan 27, 2022
@rolandschulz rolandschulz deleted the properties branch May 6, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pennycook Pennycook Pennycook approved these changes

@gmlueck gmlueck gmlueck approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

Assignees
No one assigned
Labels
Documentation Missing documentation for the code, compiler or runtime features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rolandschulz @gmlueck @bader @steffenlarsen @Pennycook