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

Adds test for the non_uniform_groups oneAPI extension #875

Conversation

steffenlarsen
Copy link
Contributor

This commit adds tests for the sycl_ext_oneapi_non_uniform_groups extension in accordance with the test plan currently in review here: #866

This commit adds tests for the [sycl_ext_oneapi_non_uniform_groups](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_non_uniform_groups.asciidoc)
extension in accordance with the test plan currently in review here:
KhronosGroup#866
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen marked this pull request as ready for review April 3, 2024 15:41
@steffenlarsen steffenlarsen requested a review from a team as a code owner April 3, 2024 15:41
size_t sub_group_size = sg.get_local_range().size();

auto fixed_size =
oneapi_ext::get_fixed_size_group<partition_size>(sg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a negative test?

Preconditions: `PartitionSize` must be less than or equal to the result of `group.get_max_local_range()`. `group.get_local_linear_range()` must be evenly divisible by `PartitionSize`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in #875 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that this cannot be a compile-time check and behavior is undefined if that precondition is not met. Unless the extensions specification is clarified, we can't have a meaningful test for that, at least in CTS

size_t sub_group_size = it.get_sub_group().get_local_range().size();

auto opportunistic =
oneapi_ext::this_kernel::get_opportunistic_group();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be checked?

Each call to `get_opportunistic_group()` returns a different group.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good question. @Pennycook - What is the intention here? Do the groups have common by-value semantics and if so, does this make the guarantee that get_opportunistic_group() != get_opportunistic_group(), which in turn means the groups will never have the same members values?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a very good question.

The intention of the wording was really to say that the members may be different, and that in cases where the members are different, the group objects may be different. Without thinking of any specific implementation: if the group members are different, the representation (e.g., as a mask) will probably be different; even if the group members are the same, I can imagine situations where the different group objects would still have dedicated scratchpad memory or barriers.

I think the simplest thing to do for now is to guarantee that equality returns false. It's probably more useful to have a comparison that is always useless than to try and define the cases in which equality should/could return true. I'd prefer not to provide a comparison operator at all, but I've heard suggestions recently that this would break a lot of things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the simplest thing to do for now is to guarantee that equality returns false. It's probably more useful to have a comparison that is always useless than to try and define the cases in which equality should/could return true. I'd prefer not to provide a comparison operator at all, but I've heard suggestions recently that this would break a lot of things.

Are you saying that opportunistic_group needs to have a comparison operator? If you are saying that things would break if there is no comparison operator, it seems surprising that those things wouldn't also break if the comparison operator always returned false.

Are you saying that the comparison operator would return false even when comparing an object with a copy? For example:

opportunistic_group og1 = /*...*/;
opportunistic_group og2 = og1;
bool same = (og1 == og2);  // returns false?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that opportunistic_group needs to have a comparison operator? If you are saying that things would break if there is no comparison operator, it seems surprising that those things wouldn't also break if the comparison operator always returned false.

I don't have complete information here. This came up in a discussion we were having about some of the other types (e.g., nd_item), and my takeaway was that that if we were to remove certain operators then the classes wouldn't play nicely with certain type traits. I might have gotten the wrong end of the stick.

Are you saying that the comparison operator would return false even when comparing an object with a copy? For example:

opportunistic_group og1 = /*...*/;
opportunistic_group og2 = og1;
bool same = (og1 == og2);  // returns false?

Yes, that's what I was suggesting.

I'm also open to defining a real comparison operator for groups, but I don't know what the criteria should be. I can see an argument that it should be based on which work-items are in the group (which would allow comparisons across groups of different types) but can also think of cases where that could lead to surprising results (e.g., just because two groups have the same work-items in them doesn't necessarily mean they can be used interchangably).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an open issue for groups in general, could we leave it out of these tests for now and address it for all groups when it has been discussed and consensus has been reached on how it should work?

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@keryell
Copy link
Member

keryell commented Apr 19, 2024

Interesting discussion about defining group equality and usability. Related to also whether we can use them in another work-group or kernel by storing them into an intermediate buffer.
To clarify for SYCL Next. :-)

tests/extension/oneapi_non_uniform_groups/CMakeLists.txt Outdated Show resolved Hide resolved
tests/extension/oneapi_non_uniform_groups/CMakeLists.txt Outdated Show resolved Hide resolved
size_t sub_group_size = sg.get_local_range().size();

auto fixed_size =
oneapi_ext::get_fixed_size_group<partition_size>(sg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that this cannot be a compile-time check and behavior is undefined if that precondition is not met. Unless the extensions specification is clarified, we can't have a meaningful test for that, at least in CTS

};
};

constexpr size_t partition_size = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could parametrize the test on the partition size and check a few different values. There are some corner cases like partition size = 1 (still a power of two! :)) and partition size = sub-group size.

I would maybe leave a TODO to expand the testing in that direction: with reqd_sub_group_size attribute it should be doable to achieve, but will require more checks to make the test portable

tests/extension/oneapi_non_uniform_groups/group_barrier.h Outdated Show resolved Hide resolved
tests/extension/oneapi_non_uniform_groups/group_barrier.h Outdated Show resolved Hide resolved
tests/group_functions/group_shift.h Outdated Show resolved Hide resolved
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen merged commit 689839c into KhronosGroup:SYCL-2020 May 30, 2024
8 checks passed
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.

6 participants