[SYCL] Enforce constraints from `sycl_ext_oneapi_reduction_properties` #16238

aelovikov-intel · 2024-12-02T21:08:00Z

No description provided.

Pennycook · 2024-12-03T09:20:36Z

sycl/include/sycl/ext/oneapi/experimental/reduction_properties.hpp


-template <typename BufferT, typename BinaryOperation, typename PropertyList>
+template <typename BufferT, typename BinaryOperation, typename PropertyList,
+          typename = std::enable_if_t<
+              detail::is_valid_reduction_prop_list<PropertyList>>>
 auto reduction(BufferT vars, handler &cgh, BinaryOperation combiner,
               PropertyList properties) {


I'm a little worried by this pattern, because it adds a template argument that is not present in the specification. I know this works, but I don't if it's legal from a SYCL specification perspective.

I'd like to hear @gmlueck's opinion about this.

In this case, it seems like you could avoid adding a new template parameter by instead adding enable_if to the handler parameter.

In general, implementing constraints seems a bit hacky in C++17 because you need to use SFINAE, and this sometimes requires adding a new template parameter or even changing a non-templated member function into a templated one. I don't know if we have a strict set of rules about whether this is completely legal. We know that changing a conversion operator from non-templated to templated changes the semantics, so we don't do this. (@aelovikov-intel has used a "mix-in" technique to avoid this for the vec class.) For the other cases, I'd advise that we avoid adding new template parameters if there is an alternative.

by instead adding enable_if to the handler parameter.

I don't remember seeing such an approach anywhere in the codebase, do you have some pointers?

For the other cases, I'd advise that we avoid adding new template parameters if there is an alternative.

I thought we're adding that extra parameter almost in every corner of the project right now...

Can't you do something like this:

template <typename BufferT, typename BinaryOperation, typename PropertyList> auto reduction(BufferT vars, std::enable_if<detail::is_valid_reduction_prop_list<PropertyList>, handler>::type &cgh, BinaryOperation combiner, PropertyList properties)

That isn't the question. What I'm saying is that I'm not aware of any place in the codebase where we use such pattern instead of extra template parameter. Why should we do anything differently here?

I guess it's just general concern that adding another template parameter might cause some code to break.

@Pennycook, @rolandschulz: Didn't we see some previous example where adding a new template parameter to accessor caused something to break, even though the template parameter had a default value? Maybe I'm misremembering?

That isn't the question. What I'm saying is that I'm not aware of any place in the codebase where we use such pattern instead of extra template parameter. Why should we do anything differently here?

FWIW, there are quite a few places (e.g., in the group algorithms) where we use enable_if on the return type instead of using an extra template parameter:

llvm/sycl/include/sycl/ext/oneapi/experimental/user_defined_reductions.hpp

Lines 46 to 49 in e9cbb87

// ---- reduce_over_group

template <typename GroupHelper, typename T, typename BinaryOperation>

std::enable_if_t<(is_group_helper_v<GroupHelper>), T>

reduce_over_group(GroupHelper group_helper, T x, BinaryOperation binary_op) {

Didn't we see some previous example where adding a new template parameter to accessor caused something to break, even though the template parameter had a default value? Maybe I'm misremembering?

I think you're referring to this issue: #10057. Even if the same problem can't actually be triggered for this overload, the fact we hit this unexpected issue with accessor does make me think that it would be safer to avoid adding extra templates just in case.

I agree with @aelovikov-intel, this is not an uncommon pattern for SFINAE on public APIs. If we believe this to be problematic, we may have to address this in other places as well, but the solution of SFINAE'ing an argument assumes that the methods have arguments, which holds in this case, but may not hold for all.

I might be wrong about this, but if we wanted to be consistent, couldn't we always implement SFINAE using the return type? Every function has a return type (but sometimes that type is void).

I appreciate it's a bit ugly to employ in a case like this where the implementation was previously relying on a deduced return type, but I think it's doable. Is there a reason that we couldn't implement this as something like:

template <typename BufferT, typename BinaryOperation, typename PropertyList> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>, detail::reduction_t<BufferT, BinaryOperation, PropertyList>> reduction(BufferT vars, handler &cgh, BinaryOperation combiner, PropertyList properties) { ... }

As an added bonus, all the SFINAE would be in a place where we could legally replace it with a requires statement when we switch to C++20:

template <typename BufferT, typename BinaryOperation, typename PropertyList> requires ValidReductionPropertyList<PropertyList> auto reduction(BufferT vars, handler &cgh, BinaryOperation combiner, PropertyList properties) { ... }

I personally prefer SFINAE on return type, with the exceptions like this one when it's deduced :) ...and ctors.

steffenlarsen · 2024-12-04T10:13:07Z

sycl/include/sycl/ext/oneapi/experimental/reduction_properties.hpp

+          typename = std::enable_if_t<
+              detail::is_valid_reduction_prop_list<PropertyList>>>
 auto reduction(BufferT vars, handler &cgh, BinaryOperation combiner,
               PropertyList properties) {


I'm tempted to say I would prefer a static_assert on whether the properties are valid. That way we can control the message telling the user why they're not allowed to make the call they're trying to do.

I don't that's valid in this case. The specification says that this is a Constraint, and ISO C++ says (emphasis mine):

[Note 1: Failure to meet such a condition results in the function's silent non-viability. — end note]

We could do this if we changed from a Constraint to a Mandate, but I don't know enough about why to favor one over the other.

Pennycook

Beautiful! Thank you for making this change, and sorry for being a pain.

aelovikov-intel · 2024-12-06T16:10:20Z

@intel/llvm-reviewers-runtime , @steffenlarsen is on vacation, can someone else review this?

uditagarwal97 · 2024-12-06T22:30:22Z

sycl/test/extensions/properties/properties_reduction.cpp

+  std::ignore =
+      sycl::reduction(r, sycl::plus<int>{},
+                      sycl_exp::properties(sycl_exp::initialize_to_identity));


Should we augment the test for deterministic property as well?

Why? The change here is about "negative" behavior, this particular line acts less like a test/check and more like "context"/"back-reference". Positive behavior tests are expected to have been added in the original PR introducing these properties.

I see. Looks good then. Also, please update the PR description to summarize the changes in this PR.

I think title already says it all. What else do you want to see there?

It is not clear from the title what "Constraints"/"Negative behavior" you are referring to.

There we go. All constraints is the keyword I'm looking for.

That's probably language barrier :) In the absence of "some" I'd just assume "all" by default :)

What's the downside of quoting the constraints...

You haven't answered my question:

What would you do if different methods had slightly different constraints? Would you list all of them separately?

I can't imagine the answer for that would be copy-pasting them all into this PR. And if we agree that we wouldn't do that for such more complex case, I can't see how a simpler one deserves a longer PR description.

What would you do if different methods had slightly different constraints? Would you list all of them separately?

Well, if the constraints were to be slightly different, we could still try to summarize them, without being overly specific.
To quote LLVM's developer policy on commit messages (https://llvm.org/docs/DeveloperPolicy.html#commit-messages):

Most importantly, the contents of the message should be carefully written to convey the rationale of the change (without delving too much in detail). It also should avoid being vague or overly specific.

we could still try to summarize them

That would be "implement constraints from ", wouldn't it?

That would be "implement constraints from ", wouldn't it?

Depends on how different the constraints are. If they are just "slightly" different, I would have come up with a better commit message than "implement constraints". If they are very different, to the extent of being unrelated, I would have tried to break PRs into smaller ones.

For the record, this is not a blocker from me since changes in SYCL RT looks good. So, if you feel strongly about not adding a commit message, please feel free to proceed with the merge.

So, if you feel strongly about not adding a commit message, please feel free to proceed with the merge.

I feel strongly about [NOT] quoting the spec :)

uditagarwal97

LGTM. Just one minor comment.

#16238)

[SYCL] Enforce constraints from sycl_ext_oneapi_reduction_properties

2020d8e

aelovikov-intel requested a review from a team as a code owner December 2, 2024 21:08

aelovikov-intel requested review from Pennycook and steffenlarsen December 2, 2024 21:08

aelovikov-intel temporarily deployed to WindowsCILock December 2, 2024 21:09 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock December 2, 2024 23:04 — with GitHub Actions Inactive

Pennycook reviewed Dec 3, 2024

View reviewed changes

steffenlarsen reviewed Dec 4, 2024

View reviewed changes

aelovikov-intel requested a review from Pennycook December 5, 2024 21:08

aelovikov-intel had a problem deploying to WindowsCILock December 5, 2024 21:09 — with GitHub Actions Error

aelovikov-intel force-pushed the reduction-property-constraints branch from b908477 to 89000af Compare December 5, 2024 21:10

aelovikov-intel had a problem deploying to WindowsCILock December 5, 2024 21:11 — with GitHub Actions Error

aelovikov-intel marked this pull request as draft December 5, 2024 21:49

Move SFINAE check to the return type

a71b1ba

aelovikov-intel force-pushed the reduction-property-constraints branch from 89000af to a71b1ba Compare December 5, 2024 21:57

aelovikov-intel temporarily deployed to WindowsCILock December 5, 2024 21:58 — with GitHub Actions Inactive

aelovikov-intel marked this pull request as ready for review December 5, 2024 22:08

aelovikov-intel temporarily deployed to WindowsCILock December 5, 2024 23:27 — with GitHub Actions Inactive

Pennycook approved these changes Dec 6, 2024

View reviewed changes

uditagarwal97 reviewed Dec 6, 2024

View reviewed changes

uditagarwal97 approved these changes Dec 6, 2024

View reviewed changes

aelovikov-intel merged commit f2c7869 into intel:sycl Dec 9, 2024
14 checks passed

aelovikov-intel deleted the reduction-property-constraints branch December 9, 2024 18:02

KornevNikita pushed a commit that referenced this pull request Feb 25, 2025

[SYCL] Enforce constraints from sycl_ext_oneapi_reduction_properties (

e1079bb

#16238)

	// ---- reduce_over_group
	template <typename GroupHelper, typename T, typename BinaryOperation>
	std::enable_if_t<(is_group_helper_v<GroupHelper>), T>
	reduce_over_group(GroupHelper group_helper, T x, BinaryOperation binary_op) {

[SYCL] Enforce constraints from sycl_ext_oneapi_reduction_properties #16238

[SYCL] Enforce constraints from sycl_ext_oneapi_reduction_properties #16238

Uh oh!

Conversation

aelovikov-intel commented Dec 2, 2024

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!

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!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Dec 6, 2024

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!

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!

uditagarwal97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[SYCL] Enforce constraints from `sycl_ext_oneapi_reduction_properties` #16238

[SYCL] Enforce constraints from `sycl_ext_oneapi_reduction_properties` #16238