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

Add test plan for oneapi_non_uniform_groups extension #866

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
370 changes: 370 additions & 0 deletions test_plans/non_uniform_groups.asciidoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,370 @@
:sectnums:
:xrefstyle: short

= Test plan for sycl_ext_oneapi_non_uniform_groups

This is a test plan for the APIs described in
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_non_uniform_groups.asciidoc[sycl_ext_oneapi_non_uniform_groups].
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved


== Testing scope

=== Device coverage

All of the tests described below are performed only on the default device that
is selected on the CTS command line.

=== Feature test macro

All of the tests should use `#ifdef SYCL_EXT_ONEAPI_NON_UNIFORM_GROUPS` so they
can be skipped if feature is not supported.

== Tests

=== The `is_fixed_topology_group` trait for existing types

Check the following:

* `is_fixed_topology_group<group<1>>::value` is `true`.
* `is_fixed_topology_group_v<group<1>>` is `true`.
* `is_fixed_topology_group<group<2>>::value` is `true`.
* `is_fixed_topology_group_v<group<2>>` is `true`.
* `is_fixed_topology_group<group<3>>::value` is `true`.
* `is_fixed_topology_group_v<group<3>>` is `true`.
* `is_fixed_topology_group<sub_group>::value` is `true`.
* `is_fixed_topology_group_v<sub_group>` is `true`.

If `SYCL_EXT_ONEAPI_ROOT_GROUP` is defined, check the following:

* `is_fixed_topology_group<root_group>::value` is `true`.
* `is_fixed_topology_group_v<root_group>` is `true`.

=== The `ballot_group` class API

The `get_ballot_group` is called with the `sub_group` of the invocation and a
predicate splitting the work-items of the sub-group into uneven groups. Let `N1`
be the size of the group created with `true` predicate and let `N2` be the size
of the group created with `false` predicate.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== Group traits

Check the following:

* `is_group<ballot_group<sub_group>>::value` is `true`.
* `is_group_v<ballot_group<sub_group>>` is `true`.
* `is_user_constructed_group<ballot_group<sub_group>>::value` is `true`.
* `is_user_constructed_group_v<ballot_group<sub_group>>` is `true`.
* `is_fixed_topology_group<ballot_group<sub_group>>::value` is `false`.
* `is_fixed_topology_group_v<ballot_group<sub_group>>` is `false`.

==== Members

Check the following:

* `id_type` is same as `id<1>`.
* `range_type` is same as `range<1>`.
* `linear_id_type` is same as `uint32_t`.
* `dimensions` is 1.
* `fence_scope` is equal to `sub_group::fence_scope`.

==== get_group_id

Check that `get_group_id()` return type is `id_type` and return value is
`1` if the predicate was `true` or `0` if the predicate was `false`.
Copy link
Member

Choose a reason for hiding this comment

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

It confused me, but this is what the specification says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ironically, the implementation and extension specification slightly disagree on this. I have a patch to make them agree: intel/llvm#12905


==== get_local_id

Check that `get_local_id()` return type is `id_type` and return value is less
than `N1` if the predicate was `true` or less than `N2` if the predicate was
`false`.

==== get_group_range

Check that `get_group_range()` return type is `range_type` and return value is
equal to `2`.

==== get_local_range

Check that `get_local_range()` return type is `range_type` and return value is
equal to `N1` if the predicate was `true` or equal to `N2` if the predicate was
`false`.

==== get_group_linear_id

Check that `get_group_linear_id()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_id()` converted to `linear_id_type`.

==== get_local_linear_id

Check that `get_local_linear_id()` return type is `linear_id_type` and the
return value is equal to `ballot.get_local_id()` converted to `linear_id_type`.

==== get_group_linear_range

Check that `get_group_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_range()` converted to `linear_id_type`.

==== get_local_linear_range

Check that `get_local_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_local_range()` converted to `linear_id_type`.

==== leader

Check that `leader()` return type is `bool` and return value is equal to
`get_local_id() == 0`.

=== The `fixed_size_group` class API

Let `N` be some power-of-two value greater than 1 that is expected to be a
Copy link
Member

Choose a reason for hiding this comment

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

Are you excluding the case of 1. That seems interesting to test corner cases, otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the class API tests we only test for one group configuration. The intention here was to pick one that wasn't 1 to avoid some trivial cases and to test a more normal group size. We could potentially run the tests for multiple configurations, if that would be more interesting? We do it for the group algorithms.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I thought you were running with several N and in that case testing also the boundaries would have made sense.

divisor of the sub-group size of most devices. The `get_fixed_size_group` is
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
called with the `sub_group` of the invocation and `N` as the partition size.
Let `M` be the size of the sub-group the given `fixed_size_group` was created
from.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== Group traits

Check the following:

* `is_group<fixed_size_group<N, sub_group>>::value` is `true`.
* `is_group_v<fixed_size_group<N, sub_group>>` is `true`.
* `is_user_constructed_group<fixed_size_group<N, sub_group>>::value` is `true`.
* `is_user_constructed_group<fixed_size_group<N, sub_group>>` is `true`.
* `is_fixed_topology_group_v<fixed_size_group<N, sub_group>>::value` is `false`.
* `is_fixed_topology_group_v<fixed_size_group<N, sub_group>>` is `false`.

==== Members

Check the following:

* `id_type` is same as `id<1>`.
* `range_type` is same as `range<1>`.
* `linear_id_type` is same as `uint32_t`.
* `dimensions` is 1.
* `fence_scope` is equal to `sub_group::fence_scope`.

==== get_group_id

Check that `get_group_id()` return type is `id<1>` and return value is less than
`M/N`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_id

Check that `get_local_id()` return type is `id<1>` and return value is less than
`N`.

==== get_group_range

Check that `get_group_range()` return type is `range_type` and return value is
equal to `M/N`.

==== get_local_range

Check that `get_local_range()` return type is `range_type` and return value is
equal to `N`.

==== get_group_linear_id

Check that `get_group_linear_id()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_id()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_linear_id

Check that `get_local_linear_id()` return type is `linear_id_type` and the
return value is equal to `ballot.get_local_id()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_group_linear_range

Check that `get_group_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_range()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_linear_range

Check that `get_local_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_local_range()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== leader

Check that `leader()` return type is `bool` and return value is equal to
`get_local_id() == 0`.

=== The `tangle_group` class API

The `get_tangle_group` is called with the `sub_group` of the invocation. This
will only be called by the first `N` items of the sub-group, where `N` is
strictly less than the size of the sub-group.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
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 it's important for the CTS to deliberately check usage of a tangle in two sides of a branch:

if (condition) {
  auto tangle = get_tangle_group(sg);
  // checks
}
else {
  auto tangle = get_tangle_group(sg);
  // identical checks
}

Compilers are incredibly likely to see code like this and try to be clever by removing the branch, but that ignores the desired tangle_group semantics. If we don't have a test like this, I don't think we can be confident that an implementation actually understands tangles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently my WIP tests use something like:

parallel_for(... [&](sycl::nd_item<1> it) {
  if (!condition)
    return;
  auto tangle = get_tangle_group(sg);
  // checks
});

Do you think it would be fine to separate them, i.e. something like:

parallel_for(... [&](sycl::nd_item<1> it) {
  if (!condition)
    return;
  auto tangle = get_tangle_group(sg);
  // checks
});

...

parallel_for(... [&](sycl::nd_item<1> it) {
  if (condition)
    return;
  auto tangle = get_tangle_group(sg);
  // identical checks
});

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't think this is enough. The behavior we want to test isn't whether tangles can exist for both condition and !condition, but rather that they can exist simultaneously. If the tests are split out, there's no risk of a compiler optimizing too aggressively.

It might be clearer if I complete my example. The concern is that a compiler may see this:

if (condition) {
  auto tangle = get_tangle_group(sg);
  // checks
}
else {
  auto tangle = get_tangle_group(sg);
  // identical checks
}

and replace it with this:

if (condition || !condition) {
  auto tangle = get_tangle_group(sg);
  // checks
}

That transformation is legal for most code, but is illegal for code containing tangle groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am of two minds regarding this. On one hand, I agree that an optimizer might elect to make that optimization, but on the other hand I don't think the CTS should make tests based on potential optimizations. An implementation could simply turn off optimizations in the CTS and pass and there could be a plethora of other patterns that could also be used to cause mayhem here, so it seems a little contrived.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see where you're coming from, but I think it's important for the CTS to test things that might break.

If an implementation has to turn off optimizations to make tangle groups properly (and pass the CTS) then they will need to communicate to users that certain functionality only works with optimizations turned off. Conversely, if the CTS doesn't test tricky cases like these, implementations that don't actually support the feature will be able to claim that they do -- and if a developer complains that the implementation's behavior is wrong, the implementation can point to the CTS as proof that it isn't!

We (DPC++) needed to run a test like this in order to find out that the initial implementation wasn't working properly. So, I think there's a good chance that having a test like this might also be useful to other implementers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. Would it be enough to do it just for the API testing? I.e. all the algorithm tests can still just use one control flow? I would assume in general having the two paths in one test should be good enough to weed out the worst cases of this kind of optimizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that's sufficient. That will give us some confidence that the groups are being constructed correctly in weird corner cases. We're already not going to test all possible code they can put in those branches, so I'm okay if we skip the algorithms too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! I have tried specifying this in the test plan.


==== Group traits

Check the following:

* `is_group<tangle_group<sub_group>>::value` is `true`.
* `is_group_v<tangle_group<sub_group>>` is `true`.
* `is_user_constructed_group<tangle_group<sub_group>>::value` is `true`.
* `is_user_constructed_group<tangle_group<sub_group>>` is `true`.
* `is_fixed_topology_group_v<tangle_group<sub_group>>::value` is `false`.
* `is_fixed_topology_group_v<tangle_group<sub_group>>` is `false`.

==== Members

Check the following:

* `id_type` is same as `id<1>`.
* `range_type` is same as `range<1>`.
* `linear_id_type` is same as `uint32_t`.
* `dimensions` is 1.
* `fence_scope` is equal to `sub_group::fence_scope`.

==== get_group_id

Check that `get_group_id()` return type is `id_type` and return value is equal
to `0`.

==== get_local_id

Check that `get_local_id()` return type is `id_type` and return value is less
than `N`.

==== get_group_range

Check that `get_group_range()` return type is `range_type` and return value is
equal to `1`.

==== get_local_range

Check that `get_local_range()` return type is `range_type` and return value is
equal to `N`.

==== get_group_linear_id

Check that `get_group_linear_id()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_id()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_linear_id

Check that `get_local_linear_id()` return type is `linear_id_type` and the
return value is equal to `ballot.get_local_id()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_group_linear_range

Check that `get_group_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_range()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_linear_range

Check that `get_local_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_local_range()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== leader

Check that `leader()` return type is `bool` and return value is equal to
`get_local_id() == 0`.

=== The `opportunistic_group` class API

The `get_opportunistic_group` is called by all work items.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
Let `M` be the size of the sub-group of the invocation.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== Group traits

Check the following:

* `is_group<opportunistic_group>::value` is `true`.
* `is_group_v<opportunistic_group>` is `true`.
* `is_user_constructed_group<opportunistic_group>::value` is `true`.
* `is_user_constructed_group<opportunistic_group>` is `true`.
* `is_fixed_topology_group_v<opportunistic_group>::value` is `false`.
* `is_fixed_topology_group_v<opportunistic_group>` is `false`.

==== Members

Check the following:

* `id_type` is same as `id<1>`.
* `range_type` is same as `range<1>`.
* `linear_id_type` is same as `uint32_t`.
* `dimensions` is 1.
* `fence_scope` is equal to `sub_group::fence_scope`.

==== get_group_id

Check that `get_group_id()` return type is `id_type` and return value is equal
to `0`.

==== get_local_id

Check that `get_local_id()` return type is `id_type` and return value is less
than `get_local_range().size()`.

==== get_group_range

Check that `get_group_range()` return type is `range_type` and return value is
equal to `1`.

==== get_local_range

Check that `get_local_range()` return type is `range_type` and return value is
less than or equal to `M`.

==== get_group_linear_id

Check that `get_group_linear_id()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_id()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_linear_id

Check that `get_local_linear_id()` return type is `linear_id_type` and the
return value is equal to `ballot.get_local_id()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_group_linear_range

Check that `get_group_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_group_range()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== get_local_linear_range

Check that `get_local_linear_range()` return type is `linear_id_type` and return
value is equal to `ballot.get_local_range()` converted to `linear_id_type`.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

==== leader

Check that `leader()` return type is `bool` and return value is equal to
`get_local_id() == 0`.

=== Group functions

The group functions

* `group_barrier`
* `group_broadcast`

for `ballot_group`, `fixed_size_group`, `tangle_group`
and `opportunistic_group` are tested similar to how they are currently tested
with `group` and `sub_group` in the core CTS. The groups are constructed in the
same way as for the API testing described above.
steffenlarsen marked this conversation as resolved.
Show resolved Hide resolved

=== Group algorithms

The group algorithms

* `joint_any_of`
* `any_of_group`
* `joint_all_of`
* `all_of_group`
* `joint_none_of`
* `none_of_group`
* `shift_group_left`
* `shift_group_right`
* `permute_group_by_xor`
* `select_from_group`
* `joint_reduce`
* `reduce_over_group`
* `joint_exclusive_scan`
* `exclusive_scan_over_group`
* `joint_inclusive_scan`
* `inclusive_scan_over_group`

for `ballot_group`, `fixed_size_group`, `tangle_group`
and `opportunistic_group` are tested similar to how they are currently tested
with `group` and `sub_group` in the core CTS. The groups are constructed in the
same way as for the API testing described above.
Loading