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.

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

Add explicit deduction guides for blocked_nd_range #1525

Merged
merged 20 commits into from
Jan 24, 2025
Merged

Add explicit deduction guides for blocked_nd_range #1525

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
eeed3f8
Enable CTAD for blocked_rangeNd since C++17
rarutyun Oct 7, 2024
3fe65dc
Fix constraits test failure
rarutyun Oct 7, 2024
ee470d8
Extend implementation with tests and explicit deduction guide
kboyarinov Oct 8, 2024
1a6717f
Remove unnecessary helper function
kboyarinov Oct 8, 2024
887e933
Simplify the deduction guide
kboyarinov Oct 8, 2024
5309e14
Add deduction guide for blocked_range type
kboyarinov Oct 8, 2024
6ccb883
Enable CTAD for blocked_rangeNd since C++17
rarutyun Oct 7, 2024
559ce1e
Fix constraits test failure
rarutyun Oct 7, 2024
64610c6
Fix merging issue
kboyarinov Dec 20, 2024
fe56f9f
Merge remote-tracking branch 'origin/rarutyun/blocked_range_nd_class'…
kboyarinov Dec 20, 2024
54b0137
Add new deduction guides and test
kboyarinov Dec 20, 2024
f45b27f
Update copyright years
kboyarinov Jan 13, 2025
1ccd670
Merge remote-tracking branch 'origin/rarutyun/blocked_range_nd_class'…
kboyarinov Jan 13, 2025
67d4a76
Update copyright years
kboyarinov Jan 13, 2025
75b60d9
Simplify implementation
kboyarinov Jan 14, 2025
ed3b67d
Save progress
kboyarinov Jan 17, 2025
7049dd2
Apply review comments + introduce preview macros
kboyarinov Jan 21, 2025
3935022
Fix fancy value
kboyarinov Jan 22, 2025
1110b0c
Add single C array support
kboyarinov Jan 22, 2025
6337734
Merge remote-tracking branch 'origin/master' into dev/kboyarinov/bloc…
kboyarinov Jan 24, 2025
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
30 changes: 30 additions & 0 deletions include/oneapi/tbb/blocked_nd_range.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,36 @@ class blocked_nd_range : public blocked_nd_range_impl<Value, N> {
using base::base;
};

#if __TBB_CPP17_DEDUCTION_GUIDES_PRESENT
template <typename Value, unsigned int... Ns,
typename = std::enable_if_t<(... && (Ns == 2 || Ns == 3))>>
blocked_nd_range(const Value (&... dim)[Ns])
-> blocked_nd_range<Value, sizeof...(Ns)>;

template <typename Value, typename... Values,
typename = std::enable_if_t<(... && std::is_same_v<Value, Values>)>>
blocked_nd_range(blocked_range<Value>, blocked_range<Values>...)
-> blocked_nd_range<Value, 1 + sizeof...(Values)>;

template <typename Value, unsigned int N,
typename = std::enable_if_t<(N != 2 && N != 3)>>
blocked_nd_range(const Value (&)[N])
-> blocked_nd_range<Value, N>;

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the argument is explicitly an array of 2 or 3 elements, not a braced init list? Seems there is no deduction guide for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing single array of 2 or 3 elements should be considered as a valid use-case, but having an extra deduction guide for array of 2 or 3 will also cover the braced-init-list of size 2 and 3.
As we discussed braced-init-list of size 2 and 3 should not be supported because of ambiguity between 1-D range constructed from single blocked_range and 2/3-D range constructed from the array.
This use-case is the main open question in the RFC #1607

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in the RFC, added support for both single array and single braced-init-list with preference to multi-dimensional ranges

template <typename Value, unsigned int N>
blocked_nd_range(const Value (&)[N], typename blocked_nd_range<Value, N>::size_type)
-> blocked_nd_range<Value, N>;

template <typename Value, unsigned int N>
blocked_nd_range(blocked_nd_range<Value, N>, oneapi::tbb::split)
-> blocked_nd_range<Value, N>;

template <typename Value, unsigned int N>
blocked_nd_range(blocked_nd_range<Value, N>, oneapi::tbb::proportional_split)
-> blocked_nd_range<Value, N>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting constructors are not really for users but for algorithms. Do we need deduction guides for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think the main purpose of the splitting constructors are TBB algorithms that don't currently use CTAD. But they are still part of the specification and the user may expect that for some implementation these constructors can be covered by the implicit deduction guides (see short example here).
And also all of the other blocked ranges in TBB currently supports CTAD for splitting constructors implicitly.
It seems like the reason why they do not work for N-d range are the class layout of the current implementation.

I feel that these deduction guides can be excluded from the future specification, but should be added into the implementation.

#endif // __TBB_CPP17_DEDUCTION_GUIDES_PRESENT

} // namespace d1
} // namespace detail

Expand Down
70 changes: 69 additions & 1 deletion test/conformance/conformance_blocked_nd_range.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright (c) 2017-2024 Intel Corporation
Copyright (c) 2017-2025 Intel Corporation

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -291,3 +291,71 @@ TEST_CASE("blocked_nd_range proportional splitting") {
utils::check_range_bounds_after_splitting(original.dim(0), first.dim(0), second.dim(0), expected_first_end);
}
}

#if __TBB_CPP17_DEDUCTION_GUIDES_PRESENT
//! Testing blocked_rangeNd deduction guides
//! \brief \ref interface
TEST_CASE("blocked_rangeNd deduction guides") {
Copy link
Contributor

Choose a reason for hiding this comment

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

The test misses any checks for constructing explicitly from an array. It needs to check construction from arrays of different sizes (incl. 2 and 3), with and without a grainsize argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

using oneapi::tbb::blocked_nd_range;

std::vector<const unsigned long*> v;
using iterator = typename decltype(v)::iterator;

oneapi::tbb::blocked_range<int> dim_range(0, 100);

blocked_nd_range<int, 2> source_range(dim_range, dim_range);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test with something more fancy than int for the value type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


{
blocked_nd_range range(dim_range, dim_range, dim_range);
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 3>>);
}
{
blocked_nd_range range({v.begin(), v.end()}, {v.begin(), v.end()});
static_assert(std::is_same_v<decltype(range), blocked_nd_range<iterator, 2>>);
}
{
blocked_nd_range range({0, 100}, {0, 100, 5}, {0, 100}, {0, 100, 5});
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 4>>);
}
{
blocked_nd_range range({100});
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 1>>);
}
{
blocked_nd_range range({100}, 5);
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 1>>);
}
{
blocked_nd_range range({100, 200}, 5);
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 2>>);
}
{
blocked_nd_range range({100, 200, 300}, 5);
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 3>>);
}
{
blocked_nd_range range({100, 200, 300, 400});
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 4>>);
}
{
blocked_nd_range range({100, 200, 300, 400}, 5);
static_assert(std::is_same_v<decltype(range), blocked_nd_range<int, 4>>);
}
{
blocked_nd_range range(source_range, oneapi::tbb::split{});
static_assert(std::is_same_v<decltype(range), decltype(source_range)>);
}
{
blocked_nd_range range(source_range, oneapi::tbb::proportional_split{1, 3});
static_assert(std::is_same_v<decltype(range), decltype(source_range)>);
}
{
blocked_nd_range range(source_range);
static_assert(std::is_same_v<decltype(range), decltype(source_range)>);
}
{
blocked_nd_range range(std::move(source_range));
static_assert(std::is_same_v<decltype(range), decltype(source_range)>);
}
}
#endif // __TBB_CPP17_DEDUCTION_GUIDES_PRESENT
Loading