-
Notifications
You must be signed in to change notification settings - Fork 109
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.
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
Added blocked_rangeNd #555
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tobias Weinzierl <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly suggesting fixes for typos, but also a couple of additional comments.
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
…ns of member functions. Align signatures in the description with the class synopsis. Some formatting fixes.
I have added |
In the last commit, I tried to specify the constructor from N blocked ranges in a different way which avoids the use of an unspecified parameter pack. @vossmjp please take a look. If this does not seem good, the only other option I could think of is uncovering the implementation trick we use. |
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
source/elements/oneTBB/source/algorithms/blocked_ranges/blocked_rangeNd_cls.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
The space is the half-open Cartesian product of ranges ``[0, size[0]) x [0, size[1]) x ...`` | ||
each having the same grain size. | ||
|
||
**Example:** The ``blocked_rangeNd<int,4> r( {5,6,7,8}, 4 );`` statement constructs a four-dimensional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other ranges constructors gives you ability to specify begins of the ranges.
At least from my perspective it less confusing to have both begins and ends explicitly specified.
Otherwise, blocked_rangeNd<int,2> r( {1,6} );
could be confused into blocked_rangeNd<int,2> r( {/*begin*/1,/*end*/6} );
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed it here #555 (comment) and agreed to only provide the "simplest" version that takes the array of sizes.
Otherwise,
blocked_rangeNd<int,2> r( {1,6} );
could be confused intoblocked_rangeNd<int,2> r( {/*begin*/1,/*end*/6} );
.
blocked_rangeNd<int,2> r( {/*begin*/1,/*end*/6} );.
should have a second dimension though to be meaningful, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have a second dimension though to be meaningful, no?
Also true but still looks a bit confusing 😄
blocked_rangeNd(const dim_range_type& dim0 /*, ... - exactly N parameters of the same type*/); | ||
blocked_rangeNd(const value_type (&size)[N], size_type grainsize = 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to address support for class template argument deduction in this PR as well, or better separately? I guess it might be non-trivial and require some experiments first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to be silent about supporting template argument deduction in this version of the spec even while developing an implementation that supports it. This would let us do the experiments etc. without committing to anything. The spec would not be incompatible with an implementation whether the implementation has support or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's OK for me. I will keep the comment open for some more time, in case others want to comment on this proposed resolution.
namespace tbb { | ||
|
||
template<typename Value, unsigned int N> | ||
class blocked_rangeNd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should blocked_rangeNd
be blocked_range_nd
? I might understand why we named blocked_range2d
and 3d
without underscore but for _nd
we have a mix of snake_case
and camelCase
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the naming question! :)
To feed the discussion: SYCL has nd_range
and nd_item
, and C++23 has mdspan
(but the real analogue to our class is called extents
in C++).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some pondering on this, I propose one more naming option to consider: blocked_md_range
. Its benefits, I think, are:
- It follows the existing naming practice (see examples in my previous comment) by putting the dimensionality marker before the noun.
- "md" stands for "multidimensional" and is consistent with C++
mdspan
. - It differs more from the current naming convention for TBB ranges, which I think makes more sense if we decide to depart from it.
- For example, if we ever want to define
blocked_2d_range<T>
as an alias toblocked_md_range<T,2>
, it will be a bit more distinct fromblocked_range2d
:)
- For example, if we ever want to define
By now, blocked_md_range
is my personal preference for the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference on a specific name. Here are my thoughts:
- I definitely do NOT like
blocked_rangend
since that is indecipherable - and I agree that
blocked_rangeNd
is not great either because it appears to be camelCase unlike any other name.
So I'm left with blocked_range_nd
, blocked_nd_range
and blocked_md_range
. Of these, I prefer the options with "nd" or "md" in the middle to differentiate even more from the existing blocked ranges. I have the slightest preference for "nd" over "md" simply because it doesn't imply multiple dimensions; it could be 1. "n" doesn't imply that it has to be "multi". But I wouldn't object to "md" if others have a preference for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not mind to use nd
in the middle of the name either.
For the sake of making progress, let's agree on blocked_nd_range
. I will update the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I would prefer blocked_nd_range as it is a proper English word then. However we have blocked_range2d and blocked_range3d, so blocked_rangeNd is more consistent!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is not blocked_rangeNd
:)
blocked_md_range
, blocked_range_md
, blocked_nd_range
, blocked_range_nd
are all fine with me, with a different (but slight) level of preference.
I slightly prefer md
to nd
. Whether this md
goes to an end to to a middle, I don't actually care. English-wise, perhaps blocked_md_range
is better. Especially if we add aliases blocked_2d_range
and blocked_3d_range
. There is a caveat with those aliases, though... While the CTAD would work for blocked_range_2d
and _3d
since C++17, it would work for the respective aliases since C++20, which (IMO) looks weird from the user standpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I would prefer blocked_nd_range as it is a proper English word then. However we have blocked_range2d and blocked_range3d, so blocked_rangeNd is more consistent!
Looks like most if not all of us are OK with some inconsistency for the sake of a better name. We can think of blocked_nd_range
as a new branch of blocked_range
evolution, not a "descendant" of 2d/3d ranges (even though it borrows from those).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can life with that as long as this rationale is documented here explicitly, i.e. we explain it in the documentation.
An n-dimensional range has been in Intel's reference implementation for quite a while. It is an absolutely essential feature for a lot of scientific codes and hence should be part of the spec. This proposal starts from Intel's reference implementation but adds an array-based constructor which we found extremely useful in our codes. A PR to the reference implementation is submitted.