Skip to content

[SYCL] Enforce constraints from sycl_ext_oneapi_reduction_properties #16238

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 9, 2024
Merged

[SYCL] Enforce constraints from sycl_ext_oneapi_reduction_properties #16238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2020d8e
[SYCL] Enforce constraints from `sycl_ext_oneapi_reduction_properties`
aelovikov-intel Dec 2, 2024
a71b1ba
Move SFINAE check to the return type
aelovikov-intel Dec 5, 2024
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
90 changes: 66 additions & 24 deletions sycl/include/sycl/ext/oneapi/experimental/reduction_properties.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,20 @@ struct initialize_to_identity_key
};
inline constexpr initialize_to_identity_key::value_t initialize_to_identity;

namespace detail {
struct reduction_property_check_anchor {};
} // namespace detail

template <>
struct is_property_key_of<deterministic_key,
detail::reduction_property_check_anchor>
: std::true_type {};

template <>
struct is_property_key_of<initialize_to_identity_key,
detail::reduction_property_check_anchor>
: std::true_type {};

} // namespace experimental
} // namespace oneapi
} // namespace ext
Expand Down Expand Up @@ -83,60 +97,88 @@ template <typename BinaryOperation>
struct IsDeterministicOperator<DeterministicOperatorWrapper<BinaryOperation>>
: std::true_type {};

template <typename PropertyList>
inline constexpr bool is_valid_reduction_prop_list =
ext::oneapi::experimental::detail::all_are_properties_of_v<
ext::oneapi::experimental::detail::reduction_property_check_anchor,
PropertyList>;

template <typename BinaryOperation, typename PropertyList, typename... Args>
auto convert_reduction_properties(BinaryOperation combiner,
PropertyList properties, Args &&...args) {
if constexpr (is_valid_reduction_prop_list<PropertyList>) {
auto WrappedOp = WrapOp(combiner, properties);
auto RuntimeProps = GetReductionPropertyList(properties);
return sycl::reduction(std::forward<Args>(args)..., WrappedOp,
RuntimeProps);
} else {
// Invalid, will be disabled by SFINAE at the caller side. Make sure no hard
// error is emitted from here.
}
}
} // namespace detail

template <typename BufferT, typename BinaryOperation, typename PropertyList>
auto reduction(BufferT vars, handler &cgh, BinaryOperation combiner,
PropertyList properties) {
PropertyList properties)
-> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>,
decltype(detail::convert_reduction_properties(
combiner, properties, vars, cgh))> {
detail::CheckReductionIdentity<typename BufferT::value_type, BinaryOperation>(
properties);
auto WrappedOp = detail::WrapOp(combiner, properties);
auto RuntimeProps = detail::GetReductionPropertyList(properties);
return reduction(vars, cgh, WrappedOp, RuntimeProps);
return detail::convert_reduction_properties(combiner, properties, vars, cgh);
}

template <typename T, typename BinaryOperation, typename PropertyList>
auto reduction(T *var, BinaryOperation combiner, PropertyList properties) {
auto reduction(T *var, BinaryOperation combiner, PropertyList properties)
-> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>,
decltype(detail::convert_reduction_properties(
combiner, properties, var))> {
detail::CheckReductionIdentity<T, BinaryOperation>(properties);
auto WrappedOp = detail::WrapOp(combiner, properties);
auto RuntimeProps = detail::GetReductionPropertyList(properties);
return reduction(var, WrappedOp, RuntimeProps);
return detail::convert_reduction_properties(combiner, properties, var);
}

template <typename T, size_t Extent, typename BinaryOperation,
typename PropertyList>
auto reduction(span<T, Extent> vars, BinaryOperation combiner,
PropertyList properties) {
PropertyList properties)
-> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>,
decltype(detail::convert_reduction_properties(
combiner, properties, vars))> {
detail::CheckReductionIdentity<T, BinaryOperation>(properties);
auto WrappedOp = detail::WrapOp(combiner, properties);
auto RuntimeProps = detail::GetReductionPropertyList(properties);
return reduction(vars, WrappedOp, RuntimeProps);
return detail::convert_reduction_properties(combiner, properties, vars);
}

template <typename BufferT, typename BinaryOperation, typename PropertyList>
auto reduction(BufferT vars, handler &cgh,
const typename BufferT::value_type &identity,
BinaryOperation combiner, PropertyList properties) {
auto WrappedOp = detail::WrapOp(combiner, properties);
auto RuntimeProps = detail::GetReductionPropertyList(properties);
return reduction(vars, cgh, identity, WrappedOp, RuntimeProps);
BinaryOperation combiner, PropertyList properties)
-> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>,
decltype(detail::convert_reduction_properties(
combiner, properties, vars, cgh, identity))> {
return detail::convert_reduction_properties(combiner, properties, vars, cgh,
identity);
}

template <typename T, typename BinaryOperation, typename PropertyList>
auto reduction(T *var, const T &identity, BinaryOperation combiner,
PropertyList properties) {
auto WrappedOp = detail::WrapOp(combiner, properties);
auto RuntimeProps = detail::GetReductionPropertyList(properties);
return reduction(var, identity, WrappedOp, RuntimeProps);
PropertyList properties)
-> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>,
decltype(detail::convert_reduction_properties(
combiner, properties, var, identity))> {
return detail::convert_reduction_properties(combiner, properties, var,
identity);
}

template <typename T, size_t Extent, typename BinaryOperation,
typename PropertyList>
auto reduction(span<T, Extent> vars, const T &identity,
BinaryOperation combiner, PropertyList properties) {
auto WrappedOp = detail::WrapOp(combiner, properties);
auto RuntimeProps = detail::GetReductionPropertyList(properties);
return reduction(vars, identity, WrappedOp, RuntimeProps);
BinaryOperation combiner, PropertyList properties)
-> std::enable_if_t<detail::is_valid_reduction_prop_list<PropertyList>,
decltype(detail::convert_reduction_properties(
combiner, properties, vars, identity))> {
return detail::convert_reduction_properties(combiner, properties, vars,
identity);
}

} // namespace _V1
Expand Down
28 changes: 28 additions & 0 deletions sycl/test/extensions/properties/properties_reduction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note %s

#include <sycl/sycl.hpp>

int main() {
int *r = nullptr;
// Must not use `sycl_ext_oneapi_reduction_properties`'s overloads:
std::ignore =
sycl::reduction(r, sycl::plus<int>{},
sycl::property::reduction::initialize_to_identity{});

namespace sycl_exp = sycl::ext::oneapi::experimental;
std::ignore =
sycl::reduction(r, sycl::plus<int>{},
sycl_exp::properties(sycl_exp::initialize_to_identity));
Comment on lines +13 to +15
Copy link
Contributor

@uditagarwal97 uditagarwal97 Dec 6, 2024

Choose a reason for hiding this comment

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

Should we augment the test for deterministic property as well?

Copy link
Contributor Author

@aelovikov-intel aelovikov-intel Dec 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@uditagarwal97 uditagarwal97 Dec 9, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@aelovikov-intel aelovikov-intel Dec 9, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@uditagarwal97 uditagarwal97 Dec 9, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@aelovikov-intel aelovikov-intel Dec 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@uditagarwal97 uditagarwal97 Dec 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@aelovikov-intel aelovikov-intel Dec 9, 2024

Choose a reason for hiding this comment

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

we could still try to summarize them

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

Copy link
Contributor

@uditagarwal97 uditagarwal97 Dec 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@aelovikov-intel aelovikov-intel Dec 9, 2024

Choose a reason for hiding this comment

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

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 :)


// Not a property list:
// expected-error@+2 {{no matching function for call to 'reduction'}}
std::ignore =
sycl::reduction(r, sycl::plus<int>{}, sycl_exp::initialize_to_identity);

// Not a reduction property:
// expected-error@+2 {{no matching function for call to 'reduction'}}
std::ignore =
sycl::reduction(r, sycl::plus<int>{},
sycl_exp::properties(sycl_exp::initialize_to_identity,
sycl_exp::full_group));
}
Loading