Skip to content

Add missing assertion to joint_of_group tests - #1240

Open
0x12CC wants to merge 1 commit into
KhronosGroup:mainfrom
0x12CC:joint-of-group-assert
Open

0x12CC wants to merge 1 commit into
KhronosGroup:mainfrom
0x12CC:joint-of-group-assert

Conversation

@0x12CC

@0x12CC 0x12CC commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

The joint_of_group tests do not work if input vector has a size of 1. This change adds an assertion to guard against work group sizes too small for the test to run correctly. The new assertion matches the intent of other similar assertions in this header. A correct SYCL implementation that uses a work group size of 1 or 2 will now fail this assertion rather than failing the test itself.

Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@0x12CC
0x12CC requested a review from a team as a code owner September 24, 2026 00:39
// greater than 2.
assert(work_group_size > 2 &&
"Not all test checks hold when the work-group size is not greater "
"than 2");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But what about the ones where the checks hold?
Are there some interesting cases where it checks for something not tested otherwise? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But what about the ones where the checks hold? Are there some interesting cases where it checks for something not tested otherwise? 🤔

This test has four predicates:

              auto none_true = [&](T i) { return i == 0; };
              auto one_true = [&](T i) { return i == 1; };
              auto some_true = [&](T i) { return i > size / 2; };
              auto all_true = [&](T i) { return i <= size; };

The predicates for one_true and some_true do not work if the input has exactly one element. This is also the motivation for the existing assertions on line 199 and line 402.

The other two predicates form valid tests even when the workgroup size is 1. I just don't think it's worth running only those tests for that input size since the larger workgroup will also test with the same predicates.

@TApplencourt

TApplencourt commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Should we skip and not assert? I mean work_group_size=1 seem to be valid as an implementation choice no?
For me assert should be about conformance precondition. skip should be for tests precondition

@0x12CC

0x12CC commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

I used assert since the other tests use assert. My experience with ProtoSYCL was that I had a test failure, because the workgroup size was 2, but the implementation was correct. I thought the assert would make it very obvious to future implementers that the test itself doesn't work in those cases.

Although a workgroup size of 1 is a valid implementation choice, my preference is to only skip tests for practical implementation choices that limit test execution. Making every test work with any strictly conformant implementation with no practical assumptions on the provided capabilities seem like a large development and maintenance effort.

@TApplencourt

Copy link
Copy Markdown
Contributor

I agree in principle about maintenance and porting effort, but in practice for this tests, it's a 10 characters change no?

@0x12CC

0x12CC commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

I agree in principle about maintenance and porting effort, but in practice for this tests, it's a 10 characters change no?

I agree, as long as we change the existing assertions to skip as well. Changing only the one I added wouldn't make sense since a workgroup size of 1 would still hit the other assertions. I don't have a strong opinion about this so I'm happy to make any change the WG agrees on. I just thought leaving the test with no changes at all would be confusing.

@TApplencourt

TApplencourt commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

. I just thought leaving the test with no changes at all would be confusing.

Totally agree. I see the confusion, sorry my bad. What I mean was to skip the tests if work_group_size not greater than 2.

I would just replace the

  assert(work_group_size > 2 &&
         "Not all test checks hold when the work-group size is not greater "
         "than 2");

with

    if !( work_group_size > 2)
      SKIP(  "Not all test checks hold when the work-group size is not greater "
         "than 2")

About supporting WG size of one, I agree it's maybe not worth the trouble of changing all the logic of this tests.

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.

3 participants