Skip to content

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

Merged
bader merged 4 commits intointel:syclfrom
rolandschulz:property_list
Oct 6, 2021
Merged

[SYCL][Doc] Add property list extension spec#4203
bader merged 4 commits intointel:syclfrom
rolandschulz:property_list

Conversation

@rolandschulz
Copy link
Contributor

@rolandschulz rolandschulz commented Jul 28, 2021

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

@keryell keryell Aug 14, 2021

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

@rolandschulz rolandschulz Sep 30, 2021

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

@keryell keryell Sep 30, 2021

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

rolandschulz commented Sep 30, 2021

@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 <michael.kinsner@intel.com>
Co-authored-by: Ronan Keryell <ronan@keryell.fr>
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 gmlueck, jbrodman and mkinsner 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

@mkinsner mkinsner Awaiting requested review from mkinsner

@jbrodman jbrodman Awaiting requested review from jbrodman

+1 more reviewer

@keryell keryell keryell left review comments

Reviewers whose approvals may not affect merge requirements

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