If we're looking to align with ISO C++ should we consider calling thisrank instead?

There is astatic constexpr rank() function that's available starting with C++23.

dimensions is already part of the SYCL 2020 group interface (in the sense that it's defined bygroup andsub_group).
I assumed we wouldn't want to remove that any time soon, so made it part of this new interface as well.

That's true, though we are adding additional interfaces to the group classes, I suspect with the intention of removing the old interfaces at some point. We could do this fordimensions as well, have an additional value rank which is equal todimensions, and then we could look to deprecatedimensions in the future.

Though saying that, this is not really an issue, just that it would be nice to align with ISO C++ naming, so it may not be worth it.

Good point, I hadn't considered thatdimensions andrank() are returning the same thing here!

I think deprecatingdimensions in the future would make sense. But we can't introduce arank value in its place, because then we'd have a variable calledrank and a function calledrank() in the same scope.rank() is how it's spelt inmdspan andextents, so once the minimum version of SYCL hits C++23 we'll be perfectly aligned.

I wonder if we should base this constrain on a type trait so that we can support additional group types withget_item in the future without having to modify the definition of this API.

I agree we should move towards a trait (or concept) eventually, but I didn't want to hold up this initial KHR with trying to define which traits we need. The IWOCL presentation and subsequent discussion suggested that we might need multiple traits (e.g., to distinguish between groups that can/cannot synchronize, groups that can/cannot be partitioned, etc).

Because of the way extensions work, I envisioned that any extension that introduces a new group and wants to be compatible with this interface could just introduce a new overload ofget_item(), rather than us having to revisit this extension and update an existing trait. Doing things this way also ensures that we don't make a function available by mistake, which I fear could be very easy if we introduce a trait too early.

So I agree we will likely end up requiring multiple traits, so we may not want this to be a generic group trait, as we may introduce a group type in the future which we don't want to supportget_item for.

Though I'm a little concerned about this approach, as it effectively means introducing new overloads for new group types which would increase the complexity, for example if we introduceget_item forwork_group andsub_group now and then later introduceroot_group,cluster_group, etc, we would end up with something like:

template <typename Group, typename = std::enable_if_t<std::is_same_v<Group, work_group> || std::is_same_v<Group, sub_group>>::type>void get_item(Group g) noexcept {}void get_item(root_group g) noexcept {}void get_item(cluster_group g) noexcept {}

Where as if we had a trait in this extension, something likeis_item_indexable (not a great name but I'm sure we could think of something better), then we could support additional types by simply making that group type conform tois_item_indexable. I feel like this would be cleaner.

template <typename Group, typename = std::enable_if_t<is_item_indexable<Group>>::type>void get_item(Group g) noexcept {}

Saying that I don't want to hold up this PR, so perhaps we could consider this as a follow up change.

We're on the same page. The original presentation that proposed that we should have a group concept calledindexable_item_group, sois_item_indexable is aligned with that proposal.

I think we should defer this conversation until a later date, though, perhaps even when we discuss the possibility of this feature becoming core. Whether we'd encode this as a concept or a set of traits will be dependent on whether we can assume C++20 as a minimum version, and that will depend on which version of SYCL this intercepts.

I also want to point out that the specification is deliberately vague about how this should be implemented, saying only:

_Constraints_: [code]#Group# is [code]#work_group# or [code]#sub_group#.

If we don't like the idea ofroot_group adding a new overload ofget_item, we could just add a relaxation that says something to the effect of: "Ifroot_group is available, theConstraints ofget_member_item are modified to permitroot_group". So I think we can achieve what you want to without exposing a user-facing trait.

Should this instead be:

Likewise for all the other functions in this KHR that take aGroup,group, orsub_group parameter by value.

I'm not opposed to this direction (and I've made the change in985a67a), but one thing we should give some thought to is if we can also fix the existing group algorithms that take a group by value.

Having overloads for bothGroup g andconst Group& g defined for the same function would lead to ambiguity, so I think the only way to fix this would be to say that the functions in SYCL 2020 takeGroup and the same functions in SYCL-Next takeconst Group&. It wouldn't make sense to deprecate these functions in between, because we're not actually removing anything.

I think all implementations of the group algorithms are header-only because they accept user-defined types, so this shouldn't be an ABI break concern. I also think that the vast majority of user would continue to work, becauseconst Group& is less restrictive thanGroup (but I'm not prepared to rule out that there might be some weird corner cases that require some adjustment).

Could we make this change in SYCL-Next, or are we stuck?

Yes, I think we need to reexamine all the APIs in the core SYCL spec that take objects by value. It probably does make sense to change them in SYCL-Next even if it does cause some weird corner cases to break. If there are cases like this, users will need to adjust in order to compile applications in SYCL-Next mode.

I'm much more concerned about retaining ABI compatibility between SYCL 2020 and SYCL-Next. It would be a major pain if vendors needed to ship two versions of every library that uses SYCL.