Skip to content

[SYCL][Doc] Add property list extension spec #4203

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
Merged
merged 4 commits into from
Oct 6, 2021
Merged

[SYCL][Doc] Add property list extension spec #4203

merged 4 commits into from
Oct 6, 2021

Conversation

rolandschulz
Copy link
Contributor

No description provided.

@rolandschulz rolandschulz requested a review from a team as a code owner July 28, 2021 07:35
@bader bader added Documentation Missing documentation for the code, compiler or runtime features, etc. spec extension All issues/PRs related to extensions specifications labels Jul 28, 2021
@bader bader changed the title Add property list extension spec [SYCL][Doc] Add property list extension spec Jul 28, 2021
@Pennycook Pennycook mentioned this pull request Aug 6, 2021
Merged
@keryell
Copy link
Contributor

keryell commented Aug 6, 2021

After looking at the concrete example, I have the feeling this could be simplified according to #4280 (comment)

@mkinsner
Copy link

mkinsner commented Aug 9, 2021

After looking at the concrete example, I have the feeling this could be simplified according to #4280 (comment)

@keryell I don't quite follow what you're suggesting could be simplified, after reading through. Can you please expand slightly?

keryell
keryell reviewed Aug 14, 2021
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

@keryell I don't quite follow what you're suggesting could be simplified, after reading through. Can you please expand slightly?

There is a complex detail description of property implementation and a layer of simplifying _v.
I am wondering whether we cannot come with a simpler API not describing the property type but just focusing on _v + some handling constexpr functions.
Then, since we no longer care about what is foo and just focus on foo_v, why no renaming foo_v just foo?

sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc
// This property has one integer non-type parameter.
struct baz {
template<int K>
using value_t = property_value<baz, std::integral_constant<int, K> >;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not directly

Suggested change
using value_t = property_value<baz, std::integral_constant<int, K> >;
using value_t = property_value<baz, K>;

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C++ doesn't (yet) support universal template template arguments and therefore we need template arguments to be always types. See also #4280 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad.

@Pennycook Pennycook mentioned this pull request Sep 2, 2021
Merged
@jbrodman jbrodman self-requested a review September 30, 2021 14:35
jbrodman
jbrodman previously approved these changes Sep 30, 2021
View reviewed changes
@rolandschulz
Copy link
Contributor Author

@keryell I don't quite follow what you're suggesting could be simplified, after reading through. Can you please expand slightly?

There is a complex detail description of property implementation and a layer of simplifying _v. I am wondering whether we cannot come with a simpler API not describing the property type but just focusing on _v + some handling constexpr functions. Then, since we no longer care about what is foo and just focus on foo_v, why no renaming foo_v just foo?

Do you agree this question was resolved as part of the discussion on #4280? Summary: we need the type for has_property.

@rolandschulz @keryell
Apply suggestions from code review
545a882 
Co-authored-by: Mike Kinsner <[email protected]>
Co-authored-by: Ronan Keryell <[email protected]>
mkinsner
mkinsner previously approved these changes Sep 30, 2021
View reviewed changes
@keryell
Copy link
Contributor

keryell commented Sep 30, 2021

Do you agree this question was resolved as part of the discussion on #4280? Summary: we need the type for has_property.

Yes, I am good.
Now I am waiting with impatience for a presentation on the big picture. :-)

keryell
keryell previously approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Interesting extension for SYCL

@bader bader dismissed stale reviews from keryell and mkinsner via 4d2651c October 6, 2021 08:14
@bader bader requested review from mkinsner, jbrodman and gmlueck October 6, 2021 08:14
@bader bader merged commit a7da8b4 into intel:sycl Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlueck gmlueck gmlueck approved these changes

@keryell keryell keryell left review comments

@mkinsner mkinsner Awaiting requested review from mkinsner

@jbrodman jbrodman Awaiting requested review from jbrodman

Assignees
No one assigned
Labels
Documentation Missing documentation for the code, compiler or runtime features, etc. spec extension All issues/PRs related to extensions specifications
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rolandschulz @keryell @mkinsner @jbrodman @gmlueck @bader