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

Jump to bottom

Added blocked_rangeNd #555

Merged
merged 20 commits into from
Oct 29, 2024
Merged

Added blocked_rangeNd #555

merged 20 commits into from
Oct 29, 2024

Conversation

tobiasweinzierl80
Copy link
Contributor

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.

@tobiasweinzierl80
Added blocked range
d402287
@tobiasweinzierl80
Adopted messages
5a1721b 
Signed-off-by: Tobias Weinzierl <tobias.weinzierl@durham.ac.uk>
@akukanov akukanov added the TBB label Aug 12, 2024
vossmjp
vossmjp reviewed Aug 13, 2024
View reviewed changes
Copy link

@vossmjp vossmjp left a 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
sasalla23 and others added 2 commits August 30, 2024 09:15
@sasalla23
Remove name of split argument in split ctor; improve description of d…
c1698d5 
…im_count
@akukanov
Add size_type to the member types. Use member types in the declaratio…
e489bac 
…ns of member functions. Align signatures in the description with the class synopsis. Some formatting fixes.
@akukanov
Copy link
Contributor

akukanov commented Sep 2, 2024

I have added using size_type = dim_range_type::size_type; to the class definition, and changed some of the methods to use it. It needs to be added to the implementation.

@akukanov
Copy link
Contributor

akukanov commented Sep 3, 2024

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.

vossmjp
vossmjp approved these changes Sep 3, 2024
View reviewed changes
Copy link

@vossmjp vossmjp left a 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.

aepanchi
aepanchi reviewed Sep 3, 2024
View reviewed changes
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
akukanov and others added 2 commits September 3, 2024 21:49
@akukanov @aepanchi
Apply suggestions from code review
07600a0 
Co-authored-by: Alexandra <alexandra.epanchinzeva@intel.com>
@akukanov @aepanchi
Address remaining review comments
73c5805 
Co-authored-by: Alexandra <alexandra.epanchinzeva@intel.com>
@akukanov akukanov mentioned this pull request Oct 14, 2024
Closed
vossmjp
vossmjp approved these changes Oct 28, 2024
View reviewed changes
Copy link

@vossmjp vossmjp left a 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.

pavelkumbrasev
pavelkumbrasev approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@pavelkumbrasev pavelkumbrasev left a comment

Choose a reason for hiding this comment

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

LGTM!

@akukanov akukanov merged commit 39c46a2 into uxlfoundation:main Oct 29, 2024
3 checks passed
This was referenced Oct 29, 2024
Open
Merged
@tobiasweinzierl80
Copy link
Contributor Author

I think this is now in great shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sasalla23 sasalla23 sasalla23 left review comments

@aepanchi aepanchi aepanchi left review comments

@rarutyun rarutyun rarutyun left review comments

@akukanov akukanov akukanov left review comments

@tobiasweinzierl tobiasweinzierl tobiasweinzierl left review comments

@pavelkumbrasev pavelkumbrasev pavelkumbrasev approved these changes

@vossmjp vossmjp vossmjp approved these changes

Assignees
No one assigned
Labels
TBB
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tobiasweinzierl80 @akukanov @tobiasweinzierl @vossmjp @pavelkumbrasev @rarutyun @aepanchi @sasalla23