[SYCL] Remove `logical_or` and `logical_and` from group algorithm tests #18411

dklochkov-emb · 2025-05-12T11:28:23Z

PR fixes tests
SYCL :: GroupAlgorithm/exclusive_scan_sycl2020.cpp
SYCL :: GroupAlgorithm/inclusive_scan_sycl2020.cpp
SYCL :: GroupAlgorithm/reduce_sycl2020.cpp

due to becoming invalid in case of binary_op is logical_or and logical_and.
Spec has Mandates for each group algorithm which tells that binary_op:
binary_op(init, *first) must return a value of type T.
etc. but logical_or and logical_and in ABI breaking mode has return type boolean

sycl/include/sycl/group_algorithm.hpp

…rithm

aelovikov-intel · 2025-05-15T16:37:25Z

sycl/include/sycl/group_algorithm.hpp

+                           bool>
+          : std::is_same_v<decltype(binary_op(binary_op_t(), binary_op_t())),
+                           binary_op_t>,
+      "Result type of binary_op must match scan accumulation type.");


must match scan accumulation type

What is the accumulation type? Can we use it directly?

As far as I understand, the accumulation type is the type of input/output data type, i.e part of typename InPtr:

using binary_op_t = std::remove_cv_t<std::remove_reference_t<decltype(*first)>>;
or
typename detail::remove_pointer<OutPtr>::type>

depending on __INTEL_PREVIEW_BREAKING_CHANGES

Can we use it directly? Or, in other words, the current condition of the static_assert doesn't match its message. Why are you changing the condition of the assert vs changing the accumulator type? How confident are you in your change?

No, it isn't. You still have std::is_same_v<decltype(binary_op(binary_op_t(), binary_op_t())), bool>. Either change the assert message (and explain why that would be a valid change!) or always compare with the actual result type.

The original problem is based on changes of return type of sycl::logical_and/sycl::logical_or.
#17239

So, when one of these structs is passed as binary operator and -fpreview-changes flag is used, the return type is not the same as the type passed to sycl::logical_and/sycl::logical_or.
That is why original assert does not work and was updated for this case

That sounds like you've treated symptoms instead of root causing the issue. The original assert had a very specific wording that doesn't take place anymore. Do you know why it was worded like that instead of a more loose wording that would have matched your changes?

@aelovikov-intel I just know why assert failed: some of binary operations return bool type instead of binary_op_t. These operations are logical_or, logical_and. I was wondering if you explain me 'why it was worded like that' and what is a new string to represent the right output if assert fails

…rithm

KseniyaTikhomirova · 2025-07-23T14:00:13Z

@dklochkov-emb @aelovikov-intel is this PR still relevant?

aelovikov-intel · 2025-07-23T15:03:38Z

@dklochkov-emb @aelovikov-intel is this PR still relevant?

The problem is relevant, but I disagree with that this PR is the proper "fix". For some reason @dklochkov-emb stopped working on it. Maybe we need to revert the original patches.

dklochkov-emb · 2025-07-23T16:48:10Z

@dklochkov-emb @aelovikov-intel is this PR still relevant?

The problem is relevant, but I disagree with that this PR is the proper "fix". For some reason @dklochkov-emb stopped working on it. Maybe we need to revert the original patches.

The original patches changed return type of some boolean operations under preview mode. We can create a separate asserts for these operations or update existing. Is there another option?

This reverts commit f3b026a.

This reverts commit 3f34653.

This reverts commit 118830c.

…rithm

…ests

dklochkov-emb · 2025-10-13T09:09:41Z

@aelovikov-intel I have removed sycl::logical_or/sycl::logical_and. Spec clearly says that group algorithm should be used only with logical operators which has return type T. If we need to use these operators with group algorithms, we need to update spec first.
Confirmed this approach with Greg.

aelovikov-intel

Title/description need to be updated. Make sure to include a link to

Spec clearly says that group algorithm should be used only with logical operators which has return type T.

I'm particularly interested how it's worded, is hard failure (static_assert) expected, or should it be SFINAE?

dklochkov-emb · 2025-10-13T14:52:45Z

Title/description need to be updated. Make sure to include a link to

Spec clearly says that group algorithm should be used only with logical operators which has return type T.

I'm particularly interested how it's worded, is hard failure (static_assert) expected, or should it be SFINAE?

Example for exclusive/inclusive scan is here in spec

Mandates: binary_op(*first, *first) must return a value of type std::iterator_traits::value_type.
...

Mandates: binary_op(init, *first) must return a value of type T.

etc.
I.e. there is no info if it should be checked with static_assert or SFINAE.

aelovikov-intel · 2025-10-13T16:43:14Z

I.e. there is no info if it should be checked with static_assert or SFINAE.

"mandates" means static_assert. You still need to update the description with something like

Those tests were invalid because the spec

 > quote
 
requires us to report an error when <...>. As such, drop the tests.

On top of that, I think we need to update our "negative" tests to verify that we indeed report an error when used like this.

dklochkov-emb · 2025-10-16T15:55:41Z

@aelovikov-intel Please, could you review again?

aelovikov-intel · 2025-10-16T22:05:42Z

sycl/test-e2e/GroupAlgorithm/logical_or_and_group_algorithms.cpp

+    cgh.parallel_for<class ExclusiveScanOverGroup>(
+        nd_range<1>(1, 1), [=](nd_item<1> it) {
+          group<1> g = it.get_group();
+          // expected-note@+1 {{in instantiation of function template specialization 'sycl::exclusive_scan_over_group<sycl::group<>, int, sycl::logical_and<int>>' requested here}}


There is an option to avoid writing all those "notes", grep the tests to check how it looks like.

aelovikov-intel · 2025-10-16T22:06:01Z

sycl/test-e2e/GroupAlgorithm/logical_or_and_group_algorithms.cpp

@@ -0,0 +1,174 @@
+// RUN: %clangxx -fsycl -Xclang -verify=expected -fpreview-breaking-changes -fsyntax-only -ferror-limit=0 %s


That file should go to sycl/test, not sycl/test-e2e. You'd also be able to simplify the test by doing device compilation only and avoiding all the queue/submit/handler/etc. Check existing negative tests there.

aelovikov-intel · 2025-10-16T22:07:21Z

sycl/test-e2e/GroupAlgorithm/logical_or_and_group_algorithms.cpp

+  TestExclusiveScanOverGroup(q);
+  TestJointExclusiveScan(q);
+  TestInclusiveScanOverGroup(q);
+  TestJointInclusiveScan(q);
+  TestReduceOverGroup(q);
+  TestJointReduce(q);


Why do we have 6 lines here but only 2 "expected-error"s?

[SYCL] fix asserts after logical operation changes

118830c

dklochkov-emb requested a review from aelovikov-intel May 12, 2025 11:28

dklochkov-emb self-assigned this May 12, 2025

dklochkov-emb requested a review from a team as a code owner May 12, 2025 11:28

dklochkov-emb temporarily deployed to WindowsCILock May 12, 2025 11:28 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock May 12, 2025 11:51 — with GitHub Actions Inactive

aelovikov-intel reviewed May 12, 2025

View reviewed changes

sycl/include/sycl/group_algorithm.hpp Outdated Show resolved Hide resolved

[SYCL] use alias for readability

3f34653

dklochkov-emb temporarily deployed to WindowsCILock May 13, 2025 10:01 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock May 13, 2025 10:23 — with GitHub Actions Inactive

Merge remote-tracking branch 'upstream/sycl' into sycl-fix-group-algo…

102248e

…rithm

dklochkov-emb temporarily deployed to WindowsCILock May 13, 2025 10:56 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock May 13, 2025 11:34 — with GitHub Actions Inactive

dklochkov-emb requested a review from aelovikov-intel May 15, 2025 16:30

aelovikov-intel reviewed May 15, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/sycl' into sycl-fix-group-algo…

cad3fa2

…rithm

dklochkov-emb temporarily deployed to WindowsCILock May 20, 2025 08:01 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock May 20, 2025 08:25 — with GitHub Actions Inactive

Merge remote-tracking branch 'upstream/sycl' into sycl-fix-group-algo…

876f979

…rithm

dklochkov-emb temporarily deployed to WindowsCILock May 21, 2025 09:32 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock May 21, 2025 10:18 — with GitHub Actions Inactive

[SYCL] use output type to check in assert

f3b026a

dklochkov-emb temporarily deployed to WindowsCILock May 22, 2025 13:06 — with GitHub Actions Inactive

dklochkov-emb requested a review from aelovikov-intel May 22, 2025 13:06

dklochkov-emb temporarily deployed to WindowsCILock May 22, 2025 13:44 — with GitHub Actions Inactive

dklochkov-emb added 5 commits October 13, 2025 10:32

Revert "[SYCL] use output type to check in assert"

76e20b6

This reverts commit f3b026a.

Revert "[SYCL] use alias for readability"

e0d9acd

This reverts commit 3f34653.

Revert "[SYCL] fix asserts after logical operation changes"

66ecb82

This reverts commit 118830c.

Merge remote-tracking branch 'upstream/sycl' into sycl-fix-group-algo…

f6a85e1

…rithm

[SYCL] do not use logical_or and logical_and in group algorithm e2e t…

aaaf763

…ests

dklochkov-emb temporarily deployed to WindowsCILock October 13, 2025 09:05 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock October 13, 2025 09:28 — with GitHub Actions Inactive

aelovikov-intel reviewed Oct 13, 2025

View reviewed changes

dklochkov-emb changed the title ~~[SYCL] fix asserts after logical operation changes~~ [SYCL] remove logical_or and logical_and from group algorithm tests Oct 13, 2025

dklochkov-emb requested a review from aelovikov-intel October 13, 2025 14:47

[SYCL] implement tests to check static assert of group algorithms

20c6e47

dklochkov-emb had a problem deploying to WindowsCILock October 16, 2025 14:19 — with GitHub Actions Error

[SYCL] fix formatting in tests

eabe3b7

dklochkov-emb temporarily deployed to WindowsCILock October 16, 2025 14:27 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock October 16, 2025 14:49 — with GitHub Actions Inactive

aelovikov-intel changed the title ~~[SYCL] remove logical_or and logical_and from group algorithm tests~~ [SYCL] Remove logical_or and logical_and from group algorithm tests Oct 16, 2025

aelovikov-intel reviewed Oct 16, 2025

View reviewed changes

aelovikov-intel requested a review from AlexeySachkov October 16, 2025 22:07

		@@ -0,0 +1,174 @@
		// RUN: %clangxx -fsycl -Xclang -verify=expected -fpreview-breaking-changes -fsyntax-only -ferror-limit=0 %s

[SYCL] Remove logical_or and logical_and from group algorithm tests #18411

Are you sure you want to change the base?

[SYCL] Remove logical_or and logical_and from group algorithm tests #18411

Uh oh!

Conversation

dklochkov-emb commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova commented Jul 23, 2025

Uh oh!

aelovikov-intel commented Jul 23, 2025

Uh oh!

dklochkov-emb commented Jul 23, 2025

Uh oh!

dklochkov-emb commented Oct 13, 2025

Uh oh!

aelovikov-intel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dklochkov-emb commented Oct 13, 2025

Uh oh!

aelovikov-intel commented Oct 13, 2025

Uh oh!

dklochkov-emb commented Oct 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[SYCL] Remove `logical_or` and `logical_and` from group algorithm tests #18411

[SYCL] Remove `logical_or` and `logical_and` from group algorithm tests #18411

dklochkov-emb commented May 12, 2025 •

edited

Loading

aelovikov-intel May 21, 2025 •

edited

Loading

aelovikov-intel left a comment •

edited

Loading