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] Remove unnecessary API from properties #13669

Open
wants to merge 4 commits into
base: sycl
Choose a base branch
from

Conversation

rolandschulz
Copy link
Contributor

@rolandschulz rolandschulz commented May 6, 2024

Removes:

  • property-key member value_t
  • property-value member key_t
  • is_property_key, is_property_value

Replaces implementation-defined with defined (clarifies spec and had no upside):

  • template order of property_value

@rolandschulz rolandschulz requested a review from a team as a code owner May 6, 2024 20:48
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.

Can you add a bit more to the PR description explaining what was removed?

I think this change looks good, but it only changes the spec. At this point, we have a bunch of properties defined already, so I presume we need to change their definitions to be in line with this new spec, right?

Also keep in mind that there is a lot of code using compile-time properties now, and we shouldn't break that code without going through some sort of deprecation process. I'm not worried about applications that define their own properties, but rather applications that use properties. I think this change will not affect these applications, but I'm mentioning the issue in case you know of some reason that applications would break.

@rolandschulz
Copy link
Contributor Author

Can you add a bit more to the PR description explaining what was removed?

will do

I think this change looks good, but it only changes the spec. At this point, we have a bunch of properties defined already, so I presume we need to change their definitions to be in line with this new spec, right?

Because the spec change doesn't change anything just removes things, we don't have to. But we can to simplify our code.

Also keep in mind that there is a lot of code using compile-time properties now, and we shouldn't break that code without going through some sort of deprecation process.

Even for experimental?

I'm not worried about applications that define their own properties

Note our current implantation doesn't allow users to define their own properties because
a) you need to specialize is_property (removed in this PR)
b) we sort based on an ID given to each property. We would need to change to sorting based on type-name. I don't think we have agreed yet whether we want to make that change.

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.

From the PR description:

Replaces implementation-defined with defined (clarifies spec and had no upside):

  • template order of property_value
  • mapping of NTTP to type

I'm not sure about these points. For the first point, I suggested different wording in the spec which keeps the template parameters of property_value unspecified.

Regarding the second point, I don't think this PR does define the mapping of the property parameters to types. We show std::integral_constant being used in some of the examples, but I assume these are just examples and not part of the spec.

@gmlueck
Copy link
Contributor

gmlueck commented May 15, 2024

I think this change looks good, but it only changes the spec. At this point, we have a bunch of properties defined already, so I presume we need to change their definitions to be in line with this new spec, right?

Because the spec change doesn't change anything just removes things, we don't have to. But we can to simplify our code.

I think we should clean up the code for two reasons. First, the DPC++ developers will likely copy the existing code patterns for "properties" rather than looking at the spec. If we don't clean up the code for the existing properties, all future properties will probably continue to use the old pattern. Second, application developers often look at the headers and assume that anything that isn't "private" or "detail" is part of the API. Therefore, we should remove things that aren't part of the specified API, or application developers will end up depending on those things anyway.

@rolandschulz rolandschulz mentioned this pull request May 16, 2024
@aelovikov-intel
Copy link
Contributor

Hi @rolandschulz , can you please resolve the conflicts so that we get this merged?

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