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

Clarify behavior when wrong property is passed #643

Open
gmlueck opened this issue Oct 16, 2024 · 12 comments
Open

Clarify behavior when wrong property is passed #643

gmlueck opened this issue Oct 16, 2024 · 12 comments

Comments

@gmlueck
Copy link
Contributor

gmlueck commented Oct 16, 2024

This question came from our development team.

The specification does not say what should happen if an incompatible property is passed to an API that takes a property_list. Here is a list of the APIs that take a property_list. I don't think any of them currently specify the behavior when an incompatible property is passed.

  • context constructors
  • queue constructors
  • buffer constructors
  • unsampled_image constructors
  • sampled_image constructors
  • accessor constructors
  • host_accessor constructors
  • local_accessor constructors
  • unsampled_image_accessor constructors
  • host_unsampled_image_accessor constructors
  • sampled_image_accessor constructors
  • host_sampled_image_accessor constructors
  • usm_allocator constructors
  • malloc_device, aligned_alloc_device
  • malloc_host, aligned_alloc_host
  • malloc_shared, aligned_alloc_shared
  • sycl::malloc, aligned_alloc
  • reduction functions
  • compile, link, build (kernel bundle)
  • stream constructors

I think there are two reasonable options:

  1. Mandate that these APIs throw errc::invalid in this case.
  2. Add a precondition that the properties are compatible with the given API, which means that the behavior is undefined if they are not.
@KseniyaTikhomirova
Copy link

KseniyaTikhomirova commented Oct 17, 2024

I want to add one more thing to consider against option #2. What to return when someone sends incorrect runtime property and we ignore it stating that it is UB but then call get/has_property with it? Then it seems to be declared as UB also but in reality it will work and user will never know that the scenario is incorrect.

@illuhad
Copy link
Contributor

illuhad commented Oct 29, 2024

Third option, which I think is probably most inline with what is happening with current implementations:

If property is for a different API and meaningless for the one it is passed to, define that it will be ignored (not UB).

This would allow reusing property lists, i.e. something like the following:

property_list props {property::queue::in_order{}, property::buffer::use_host_ptr{}};

sycl::queue q{props};
sycl:: buffer<...> b {..., props};

which could perhaps be convenient to specify the desired behavior of the program more globally.

If we don't want to support that use case, then I think there is little value in having a generic property_list that is independent from the type of the API that it carries properties for. In that case, I think the better solution would be to have queue_property_list, buffer_property_list etc, which could already check at compile-time when they are constructed whether the properties passed to them are the right ones.

@gmlueck
Copy link
Contributor Author

gmlueck commented Oct 30, 2024

If we don't want to support that use case, then I think there is little value in having a generic property_list that is independent from the type of the API that it carries properties for. In that case, I think the better solution would be to have queue_property_list, buffer_property_list etc, which could already check at compile-time when they are constructed whether the properties passed to them are the right ones.

It seems wrong to me to provide a guarantee that an incompatible property is ignored. I think this would most likely indicate a bug in the application, and I think it should be valid to diagnose an error.

Commenting on this last part, though. In fact, our compile-time property extension would provide the behavior you are suggesting, with a separate property list types. With compile-time properties. the property list type is a template that depends on the properties that it contains. As a result, you can diagnose incompatible properties at compile time.

@illuhad
Copy link
Contributor

illuhad commented Oct 31, 2024

Commenting on this last part, though. In fact, our compile-time property extension would provide the behavior you are suggesting, with a separate property list types. With compile-time properties. the property list type is a template that depends on the properties that it contains. As a result, you can diagnose incompatible properties at compile time.

Do you error already when the property list is constructed with properties for multiple APIs, or only when you use the property list in an API?
My proposal was for the former case. And I think that this case calls into question whether there should be a unified property_list type at all, since effectively what you do is that you introduce a queue_property_list, buffer_property_list etc.

@gmlueck
Copy link
Contributor Author

gmlueck commented Oct 31, 2024

Do you error already when the property list is constructed with properties for multiple APIs, or only when you use the property list in an API?

No, we do not error in this situation, and I'm not proposing that we do. I think it should be legal to construct a property list with any combination of properties, even if that combination cannot be passed to any of the SYCL APIs. As you suggest, I only think we should diagnose an error if the property list is passed to an API and the list contains a property that is incompatible with that API.

My proposal was for the former case. And I think that this case calls into question whether there should be a unified property_list type at all, since effectively what you do is that you introduce a queue_property_list, buffer_property_list etc.

I definitely think the existing property_list should be improved, and I think compile time properties solve our problems here. I don't think we need to hard-code specific property list types for each SYCL API that takes a property list.

@illuhad
Copy link
Contributor

illuhad commented Oct 31, 2024

No, we do not error in this situation, and I'm not proposing that we do. I think it should be legal to construct a property list with any combination of properties, even if that combination cannot be passed to any of the SYCL APIs. As you suggest, I only think we should diagnose an error if the property list is passed to an API and the list contains a property that is incompatible with that API.

What is the use case for being able to construct property lists of arbitrary properties, if you then cannot pass those to APIs?
I'm not sure I see the value here. If having such property lists is an important use case, then forcing users to disentangle the properties for different APIs seems pretty inconvenient.

So my feeling is that we should either:

  • require that incompatible properties be ignored by APIs, or
  • catch the situation as early as possible by preventing such property lists from being constructed in the first place. In this case, we should also consider having separate property list types for each API.

@gmlueck
Copy link
Contributor Author

gmlueck commented Oct 31, 2024

  • catch the situation as early as possible by preventing such property lists from being constructed in the first place. In this case, we should also consider having separate property list types for each API.

Adding separate property list types is out of scope for SYCL 2020. Given that we have a single property_list in SYCL 2020, I think it would be difficult / impossible to diagnose incompatibilities at the point the property_list was constructed.

Why are you opposed to throwing an exception at the point when we can diagnose an error?

@illuhad
Copy link
Contributor

illuhad commented Oct 31, 2024

Adding separate property list types is out of scope for SYCL 2020.

Agreed.

Given that we have a single property_list in SYCL 2020, I think it would be difficult / impossible to diagnose incompatibilities at the point the property_list was constructed.

At the very least, I suppose we could throw an exception at runtime when the property list constructor runs. But there would be some runtime overhead due to having to runtime-type-check the properties.
But looking at the spec, property_list only has a template constructor - so couldn't we already in SYCL 2020 implement the constructor with some enable_if magic such that it only works if all properties passed to it are for the same API?

Why are you opposed to throwing an exception at the point when we can diagnose an error?

I'm not super hard against it, I'm just trying to figure out what the use cases are that we think are important for property list, and where we want to go with property list in the future. In that context, depending on what we decide regarding the use cases, I'm not sure it's the best solution. For example, if we think that property list of arbitrary, incompatible types is important, we should ignore incompatible properties in our APIs. If we say that it's crucial that such a case is prevented, we should do it as early as possible.

@gmlueck
Copy link
Contributor Author

gmlueck commented Nov 1, 2024

But looking at the spec, property_list only has a template constructor - so couldn't we already in SYCL 2020 implement the constructor with some enable_if magic such that it only works if all properties passed to it are for the same API?

I think this would be hard because some properties are valid for more than one SYCL API. In any case, this wouldn't diagnose the case when you created a property list with consistent properties that was passed an an incompatible API. Consider the common case of a property list with one property. Obviously, that one property is consistent with itself, so the property list constructor won't diagnose an error. However, the application could still pass that property list to an API which doesn't accept that property.

@KseniyaTikhomirova
Copy link

@illuhad, @gmlueck kindly ping about this issue, we do not have an agreement yet.
Thank you.

@gmlueck
Copy link
Contributor Author

gmlueck commented Dec 4, 2024

It seems like discussion of a third option has petered out, so I think we are back to the two options I proposed at the beginning:

  1. Mandate that these APIs throw errc::invalid in this case.
  2. Add a precondition that the properties are compatible with the given API, which means that the behavior is undefined if they are not.

In both cases, it would be valid for the implementation to throw an exception from the constructor if the property list contains incompatible properties. Therefore, the only difference is whether the exception is mandated.

I have a slight preference for (2) only because it will allow the implementation to skip the error check in cases where it is on the hot path for launching a kernel. (For example, this might be the case for an accessor constructor.) Implementations could provide an option to enable the checking in such cases. This is only a slight preference, though, and I could be persuaded that (1) is better if we think the error check will not add significant overhead. (@KseniyaTikhomirova maybe you have insight here?)

I think the problem will go away when we adopt compile-time properties because then we will be able to diagnose incompatible properties at compile time (with no runtime overhead).

@keryell
Copy link
Member

keryell commented Dec 5, 2024

I vote for 1. throwing errc::invalid.

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

No branches or pull requests

4 participants