Unify group code snippet with other non-user-constructible classes

[AlexeySachkov]

group is not user-constructible, but explicit information about its constructors was missing from the code snippet.

[gmlueck]

Adding the editorial label. The spec was already clear that these types are not user constructible, so this PR just clarifies that in the synopsis. I'll wait a few days before merging in case others have comments.

[keryell]

Just curious about this implementation detail leakage: I have the feeling that "not user-constructible" does not mean it has no default constructor, just it cannot be constructed by the user. For example the constructor could be private.

[gmlueck]

Just curious about this implementation detail leakage: I have the feeling that "not user-constructible" does not mean it has no default constructor, just it cannot be constructed by the user. For example the constructor could be private.

This situation occurs in several places in the SYCL spec. We want to document that one of the implicitly-defined member functions is not available to the application. We currently shows these cases as = delete in the code synopsis. You are correct, though, that an implementation could make these member functions unavailable in other ways, though. How does the C++ specification handle cases like this? Is there an example of a standard C++ class whose default constructor is not available? How does the C++ specification illustrate this?

There is a similar situation with std::atomic_ref, where the copy-assignment operator is not available. The C++ specification documents the operator as = delete. Does this mean that an implementation must actually delete the operator, or could an implementation make the operator unavailable in some other way (such as declaring it private)?

[Pennycook]

I agree with Ronan here. We need to be precise about what "not user constructible" means, and I think it's different to saying that the default constructor is deleted.

I ran some experiments (see https://godbolt.org/z/d1h4o8Ge3), and the results are a little surprising. clang doesn't complain about the construction of b below in C++17, but does in C++20.

struct S2 {
    S2() = delete;
};
S2 a; // this is invalid
S2 b{}; // this is (surprisingly) valid, even though the constructor is deleted

struct S3 {
    private:
    S3() {};
};
S3 c; // this is invalid
S3 d{}; // this is also invalid

Does anybody understand this behavior?

Regardless, I think that in order to accept = delete as the solution here, we need to convince ourselves: 1) that it is sufficient to prevent users from constructing group objects; and 2) that we want to forbid implementations from defining private default constructors for group objects.

[nliber]

@Pennycook There was a core issue on this (I can't check at the moment because open-std.org is down).

The reason it compiles in C++17 is because S2 b{}; is doing aggregate initialization and ignoring the default constructor. This got fixed in C++20.

[Pennycook]

@Pennycook There was a core issue on this (I can't check at the moment because open-std.org is down).

The reason it compiles in C++17 is because S2 b{}; is doing aggregate initialization and ignoring the default constructor. This got fixed in C++20.

Oof. Does that mean that we can't use = delete to specify this in SYCL 2020, but we could in a future version of SYCL (if it adopted C++20 as the minimum language version)?

[nliber]

I think we can use = delete;. None of the real types are actually aggregates, are they? Specifically, while the definition of aggregate has changed over the years, anything with at least one non-public non-static data member has never been an aggregate.

We might have to call out that these types are not aggregates.

[Pennycook]

None of the real types are actually aggregates, are they?

I don't think they're required to be, but I think they currently can be.

After the discussion in #532, we reached the conclusion that classes like nd_item and group could legally be stateless (i.e., they don't need to store the local range, etc, but can query it directly with an intrinsic). The DPC++ implementation of nd_item is now stateless and has no data members. I might be wrong, but I think it would be an aggregate if not for the fact that it declares a protected default constructor as an implementation detail.

We might have to call out that these types are not aggregates.

Would we just say something like "These types must not be aggregates." and leave it up to individual implementations to decide how to enforce that? I can imagine some implementations will want to rely on private/protected members, while others will want to rely on a private/protected constructor.

[keryell]

After the discussion in #532, we reached the conclusion that classes like nd_item and group could legally be stateless (i.e., they don't need to store the local range, etc, but can query it directly with an intrinsic). The DPC++ implementation of nd_item is now stateless and has no data members. I might be wrong, but I think it would be an aggregate if not for the fact that it declares a protected default constructor as an implementation detail.

Stateless means empty, which does not seem like an aggregate to me, since in C++ it will have a size of 1 anyway to enforce uniqueness of objects.

Otherwise, I was looking at some types in C++. For example https://en.cppreference.com/w/cpp/types/type_info describes the constructor as deleted but looking more carefully at https://eel.is/c++draft/type.info it is just that at least there is no exposed public constructor.

At the end, this is not super important. We could always have a public constructor = delete and have some other private constructor with some tag-dispatch.

[nliber]

> Would we just say something like "These types must not be aggregates." and leave it up to individual implementations to decide how to enforce that?

I think so, especially since this is really just an issue for those compilers supporting C++17. Any version of SYCL that requires C++20 or later will likely just delete the default constructor.

[AlexeySachkov]

Just curious about this implementation detail leakage: I have the feeling that "not user-constructible" does not mean it has no default constructor, just it cannot be constructed by the user. For example the constructor could be private.

Looking into 16.3.2.4 Detailed specifications it says the following:

Descriptions of class member functions follow the order (as appropriate): 138
...
138) To save space, items that do not apply to a class are omitted. For example, if a class does not specify any comparison operator functions, there will be no “Comparison operator functions” subclause.

This makes me think that we should actually omit the description of any constructors to say that there aren't any (user-visible) constructors.

[keryell]

There is a similar situation with std::atomic_ref, where the copy-assignment operator is not available. The C++ specification documents the operator as = delete. Does this mean that an implementation must actually delete the operator, or could an implementation make the operator unavailable in some other way (such as declaring it private)?

std::atomic_ref is very different because it defines a reference wrapper, so a default constructor does not make sense, even internally.

[tomdeakin]

WG suggests we change the spec to say not default-constructible and add comment to synopsis that default constructor is not available.

[Pennycook]

WG suggests we change the spec to say not default-constructible and add comment to synopsis that default constructor is not available.

Following up offline, as discussed...

I agree with this, but I still think we should go one step further and also say either:

• group is not an aggregate; or
• group is not default-initializable.

There are good reasons to allow SYCL implementations to define things like private/protected constructors (because they need to make the object somehow!). I don't think there is a good reason to allow SYCL implementations to (perhaps accidentally) implement group in a way that allows developers to create a group without going through one of the standard SYCL functions.

If group g{} compiles and works with one SYCL implementation, it is almost certain that somebody somewhere will come to rely on that. See Hyrum's Law. I understand this would likely only cause problems for that specific implementation and may therefore not be an issue with the specification per se, but since many of us are implementers too, I think it makes sense to close as many these gaps as we can.

[etomzak]

I'm very interested in the conclusion to the non-user-constructible wording problem.

I agree with this, but I still think we should go one step further and also say either:

* `group` is not an aggregate; or

* `group` is not default-initializable.

"Not default-initializable" makes more sense to me. "Aggregate" is more like an implementation detail; "initializable" describes a user-facing interface.

There are good reasons to allow SYCL implementations to define things like private/protected constructors (because they need to make the object somehow!). I don't think there is a good reason to allow SYCL implementations to (perhaps accidentally) implement group in a way that allows developers to create a group without going through one of the standard SYCL functions.

I'd take it one step further. If the SYCL spec literally contains the code group() = delete;, then that strongly suggests that a conformant implementation must also contain that code. IMO group() = delete; is a misdirect, because it doesn't prevent all user actions that the spec wants to disallow, but it does prevent some things that the spec wants an implementation to be able to do.

If group g{} compiles and works with one SYCL implementation, it is almost certain that somebody somewhere will come to rely on that. See Hyrum's Law. I understand this would likely only cause problems for that specific implementation and may therefore not be an issue with the specification per se, but since many of us are implementers too, I think it makes sense to close as many these gaps as we can.

I think the key question is, what should happen when a user does group g{}? Is it UB? Is it ill-formed? Erroneous? Something gets thrown somewhere? Something else entirely? To avoid Hyrum's Law, ill-formed (with a diagnostic) might be the way to go. Perhaps that's all the SYCL spec needs to say, and it'll be up to the SYCL implementation to figure out how to emit an appropriate diagnostic.

[gmlueck]

Illustrating my opinion using item as an example. The current specification has this wording and synopsis:

Instances of the item class are not user-constructible and are passed by the runtime to each instance of the function object.

[...]

template <int Dimensions = 1, bool WithOffset = true> class item {
 public:
  item() = delete;
  // ...
};

I suggest we change this to something along these lines:

The item class is neither default constructible nor default initializable. The SYCL implementation typically creates these objects when invoking a kernel, passing an item object as a parameter to the kernel function.

[...]

template <int Dimensions = 1, bool WithOffset = true> class item {
 public:
  // Default constructor not available
  // ...
};

My interpretation of this is that a SYCL implementation is not required to provide a default constructor or to allow item to be default initialized. Therefore, applications that do this are not conformant. I think that technically an implementation could provide a default constructor (e.g. as an extension) if it wanted. Therefore, I think this spec wording is not a mandate that an implementation must diagnose an error if an application attempts to default-construct or default-initialize an item.

[keryell]

I'm very interested in the conclusion to the non-user-constructible wording problem.
[...]
I think the key question is, what should happen when a user does group g{}? Is it UB? Is it ill-formed? Erroneous? Something gets thrown somewhere? Something else entirely? To avoid Hyrum's Law, ill-formed (with a diagnostic) might be the way to go. Perhaps that's all the SYCL spec needs to say, and it'll be up to the SYCL implementation to figure out how to emit an appropriate diagnostic.

This should not compile at least.
By looking ahead at C++26 which allows

group() = delete("The SYCL 2032 specification does not allow default construction because we care a lot for your safety");

I am in favor of using = delete() since the aggregate initialization will be fixed too.
In the mean time...

[Pennycook]

My interpretation of this is that a SYCL implementation is not required to provide a default constructor or to allow item to be default initialized. Therefore, applications that do this are not conformant. I think that technically an implementation could provide a default constructor (e.g. as an extension) if it wanted. Therefore, I think this spec wording is not a mandate that an implementation must diagnose an error if an application attempts to default-construct or default-initialize an item.

@gmlueck, I agreed with you up until this point.

I think a reasonable C++ developer would expect the phrasing "The item class is neither default constructible nor default initializable." to be equivalent to "std::is_default_constructible_v<T> is false and T does not satisfy std::default_initializable". Whether a class satisfies these traits/concepts is user-observable, and has observable side-effects: if we define another SYCL feature and say "it only accepts default-initializable types", we want that statement to forbid accepting the group type. Allowing extensions to change statements like this seems like a bad idea, and I think if we add phrasing like this it should be backed up by CTS tests that ensure conformant implementations return the expected value(s) for the related traits.

[etomzak]

Whether a class satisfies these traits/concepts is user-observable, and has observable side-effects: if we define another SYCL feature and say "it only accepts default-initializable types", we want that statement to forbid accepting the group type.

Agree with this ^

Allowing extensions to change statements like this seems like a bad idea, and I think if we add phrasing like this it should be backed up by CTS tests that ensure conformant implementations return the expected value(s) for the related traits.

I'm not sure what to make of this. Are you saying that there are some requirements that the spec places on an implementation that a vendor extension is allowed to override, and there are other requirements that a vendor extension is not allowed to override? How does (can?) the spec capture these two classes of requirements?

[Pennycook]

I'm not sure what to make of this. Are you saying that there are some requirements that the spec places on an implementation that a vendor extension is allowed to override, and there are other requirements that a vendor extension is not allowed to override? How does (can?) the spec capture these two classes of requirements?

To be honest, I haven't thought about this a lot until now, so I'm not sure how to express my opinion precisely. I think my feelings are roughly in line with the extension guidelines, but I might not be interpreting them correctly. They could be clearer.

sycl::atomic_ref defines the set of types that can be used with the class template. An extension that adds a new specialization of sycl::atomic_ref is overriding a restriction, but does so without changing "the definition of existing functions defined by the core SYCL specification in a way that changes their specified behavior".

The restrictions on device code are a weaker example. The specification says that certain things (e.g., virtual functions) are "not permitted", but doesn't specify the behavior of virtual functions. My interpretation of the guidelines allows an extension to add support for virtual functions (overriding a restriction), without changing specified behavior. (If the specification had said that implementations must fail to compile device code containing virtual functions, I think it would be reasonable to require an implementation to pass a CTS test checking that; an implementation would have to disable the extension to be conformant.)

I feel especially strongly when it comes to statements about type traits and concepts, because any change to those changes specified behavior. Whether we're asserting that a class must or must not satisfy a certain trait/concepts, I don't see any point specifying the behavior if extensions can override it. We shouldn't encourage developers to use these traits/concepts if they're unreliable, because it might break overload resolution in some spectacular ways. If we want to give implementations wiggle room in a case like this, we should either say nothing or explicitly state that certain behavior is "not guaranteed".

[gmlueck]

Perhaps this language captures my intent better, though it is more wordy.

The SYCL specification does not provide a default constructor for item nor does it guarantee that item can be default initialized. A SYCL implementation typically creates these objects when invoking a kernel, passing an item object as a parameter to the kernel function.

[...]

template <int Dimensions = 1, bool WithOffset = true> class item {
 public:
  // Default constructor not available
  // ...
};

Responding to @keryell: It does seem nice for users to require all implementations to delete the default constructor. However, I thought we rejected this because it would prevent an implementation from defining the default constructor as a private member function.