64-bit indexing for DeviceSegmentedReduce #414

jecs · 2023-09-07T18:03:02Z

Description

Added NumSegmentsT parameter to DeviceSegmentedReduce methods, so now num_segments can be of any integral type
Added OffsetT type resolution in a manner similar to DeviceReduce: set to std::common_type_t of BeginOffsetIterT::value_type and EndOffsetIterT::value_type. (see detail/choose_offset.cuh)
In a manner similar to DeviceReduce, we use static_cast to convert num_segments to OffsetT before dispatching
OffsetT type resolution uses std::common_type_t, std::void_t, and one inline constexpr variable (is_iterator_v).

Checklist

New or existing tests cover these changes.
The documentation is up to date with these changes.

* Added NumSegmentsT parameter to methods, so now num_segments can be of any integral type * Added OffsetT type resolution in a manner similar to DeviceReduce: set to std::common_type_t of BeginOffsetIterT::value_type and EndOffsetIterT::value_type. (see detail/choose_offset.cuh) * In a manner similar to DeviceReduce, we use static_cast to convert num_segments to OffsetT before dispatching * OffsetT type resolution uses std::common_type_t, std::void_t, and one inline constexpr variable (is_iterator_v).

copy-pr-bot · 2023-09-07T18:03:05Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

jecs · 2023-09-07T18:05:28Z

Hi, should I create an Issue and add it to the description? I originally submitted this PR to the old Cub repository.

gevtushenko

Thank you for the contribution! I've left a few comments, but before proceeding with the review, we'll need a test case that covers 64-bit problem sizes. Could you please add one? In order to keep moderate memory usage, you could try to rely on counting or constant iterators instead of actually allocating huge inputs.

cub/cub/device/dispatch/dispatch_reduce.cuh

cub/cub/detail/choose_offset.cuh

jecs · 2023-09-08T02:08:49Z

Thank you for the contribution! I've left a few comments, but before proceeding with the review, we'll need a test case that covers 64-bit problem sizes. Could you please add one? In order to keep moderate memory usage, you could try to rely on counting or constant iterators instead of actually allocating huge inputs.

Thank you for the feedback! Yes, sure thing. I'll write a quick test using some kind of counter iterator- or constant iterator-based approach.

miscco

Thanks a lot for the contribution, I am so sorry about our support situation

cub/cub/detail/choose_offset.cuh

* Removed NumSegmentsT portion * Added catch2 test * Modified previous test slightly to use correct type for num_segments * Made type deduction SFINAE-friendly

jecs · 2023-09-11T20:44:23Z

@miscco @senior-zero Ready. (I hope.) I walked back 64-bit num_segments. I hope to submit that in a new Pull Request in the near future. For the test, I simply copied the previous test and modified it slightly.

cub/test/catch2_test_device_segmented_reduce_iterators_64bit.cu

…e-reduce-64bit

miscco · 2023-09-13T06:24:22Z

/ok to test

jecs · 2023-09-13T13:36:40Z

Whoops! I deleted the offending lines and ran C++11 tests on my end. Everything seems to work now. Getting your code to be compliant with several standards is tricky business.

miscco · 2023-09-13T15:18:36Z

/ok to test

jecs · 2023-09-13T17:15:57Z

All set?

gevtushenko · 2023-09-13T18:22:15Z

All set?

Not yet, I have a few comments. Will complete the review soon.

gevtushenko

Thank you for the work! There are a few minor changes that has to be made before we merge this.

cub/test/catch2_test_device_segmented_reduce_iterators_64bit.cu

cub/cub/detail/choose_offset.cuh

cub/test/catch2_test_device_segmented_reduce.cu

cub/test/catch2_test_device_segmented_reduce_iterators_64bit.cu

* Incorporated suggested changes to cub/test/catch2_test_device_segmented_reduce_iterators_64bit.cu and greatly simplified the test itself. * Ran clang-format of LLVM 16 using cub/.clang-format on modified files

jecs · 2023-09-14T19:08:28Z

So, did I misuse clang-format? It seems to have changed a lot more than expected, but the files look clean. It's my first time using the command. Are there options to avoid formatting comments? I used a standalone clang-format binary from LLVM 16.

cub/cub/device/device_segmented_reduce.cuh

gevtushenko · 2023-09-19T20:13:57Z

@jecs your repo seems to be protected. I've filed a PR with a few fixes: jecs#1 for your branch

Device reduce 64bit

gevtushenko · 2023-09-19T20:42:31Z

/ok to test

jecs · 2023-09-21T19:19:26Z

@senior-zero I added you as a collaborator on my fork in case you'd like to make more changes.

gevtushenko · 2023-09-22T15:07:14Z

/ok to test

jecs · 2023-09-27T14:47:45Z

@senior-zero @miscco Thank you both so much for your excellent feedback and for guiding me through the process! I learned a lot from this experience. It's my first time submitting a pull request to a public project like this. And thank you for teaching me about the "forbidden fruit" hidden within ::detail and also for taking the wheel towards the end.

gevtushenko · 2023-09-27T15:04:59Z

@jecs thank you for the contribution!

jecs requested review from a team as code owners September 7, 2023 18:03

jecs requested review from elstehle and griwes and removed request for a team September 7, 2023 18:03

jecs added 5 commits September 7, 2023 15:18

Fixed minor mistake in OffsetT static_assert

525aef9

Fixed type checking in choose_offset.cuh

6905d7f

Undid erroneous changes due to using old Cub repository

2b9e144

Undid the last few erroneous changes

e3d3ec6

Converted tabs to spaces

34c733d

gevtushenko reviewed Sep 8, 2023

View reviewed changes

cub/cub/device/dispatch/dispatch_reduce.cuh Outdated Show resolved Hide resolved

cub/cub/detail/choose_offset.cuh Outdated Show resolved Hide resolved

cub/cub/detail/choose_offset.cuh Outdated Show resolved Hide resolved

miscco requested changes Sep 8, 2023

View reviewed changes

64-bit indexing for DeviceSegmentedReduce

5b82798

* Removed NumSegmentsT portion * Added catch2 test * Modified previous test slightly to use correct type for num_segments * Made type deduction SFINAE-friendly

jecs and others added 2 commits September 11, 2023 16:54

fixed minor formatting issue

d7a7ed4

Merge branch 'NVIDIA:main' into device-reduce-64bit

5f694d7

gevtushenko reviewed Sep 11, 2023

View reviewed changes

cub/test/catch2_test_device_segmented_reduce_iterators_64bit.cu Outdated Show resolved Hide resolved

jecs added 3 commits September 11, 2023 20:28

Made test input sequences with number of elements >= 2^32

a4c032b

Merge branch 'device-reduce-64bit' of github.com:jecs/cccl into devic…

39bad5d

…e-reduce-64bit

Removed some cruft

cfef155

jecs requested a review from miscco September 12, 2023 20:56

Removed defaulted assignment operators for C++11 compatibility

129b95c

jecs requested a review from gevtushenko September 13, 2023 13:43

miscco approved these changes Sep 13, 2023

View reviewed changes

gevtushenko requested changes Sep 13, 2023

View reviewed changes

New test + clang-format on modified files

f94bf3a

* Incorporated suggested changes to cub/test/catch2_test_device_segmented_reduce_iterators_64bit.cu and greatly simplified the test itself. * Ran clang-format of LLVM 16 using cub/.clang-format on modified files

jecs requested a review from gevtushenko September 14, 2023 19:27

Test signed offset types in segmented reduce

31554f8

gevtushenko requested changes Sep 19, 2023

View reviewed changes

cub/cub/device/device_segmented_reduce.cuh Outdated Show resolved Hide resolved

Return old offset type to segmented arg reductions

46dc4c8

Merge pull request #1 from senior-zero/device-reduce-64bit

b598341

Device reduce 64bit

jecs requested a review from gevtushenko September 20, 2023 18:49

gevtushenko approved these changes Sep 21, 2023

View reviewed changes

Merge branch 'main' into device-reduce-64bit

a270c0d

gevtushenko merged commit 94b358b into NVIDIA:main Sep 27, 2023

jecs deleted the device-reduce-64bit branch September 28, 2023 15:57

gevtushenko mentioned this pull request Dec 15, 2023

[BUG]: Improve error reporting for segmented reduce #1221

Closed

1 task

elstehle mentioned this pull request Feb 21, 2024

[EPIC]: CUB large input support #50

Open

25 tasks

gonidelis mentioned this pull request Mar 6, 2024

[BUG]: cuda::std::iterator_traits does not expose proper member types in old C++ dialects #1509

Closed

1 task

elstehle mentioned this pull request Aug 15, 2024

Describe potential solutions for algorithm implementations and their pros/cons #1771

Closed

elstehle mentioned this pull request Oct 9, 2024

Add support for large num_items to DeviceReduce::{ArgMin,ArgMax} #2515

Closed

6 tasks

elstehle mentioned this pull request Oct 29, 2024

Adds support for large num_items to DeviceReduce::{ArgMin,ArgMax} #2647

Merged

2 tasks

elstehle mentioned this pull request Dec 12, 2024

[FEA]: Introduce new DeviceReduce::Arg{Min,Max} interface with two output iterators #3146

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

64-bit indexing for DeviceSegmentedReduce #414

64-bit indexing for DeviceSegmentedReduce #414

jecs commented Sep 7, 2023 •

edited by gevtushenko

Loading

copy-pr-bot bot commented Sep 7, 2023

jecs commented Sep 7, 2023 •

edited

Loading

gevtushenko left a comment

jecs commented Sep 8, 2023 •

edited

Loading

miscco left a comment

jecs commented Sep 11, 2023

miscco commented Sep 13, 2023

jecs commented Sep 13, 2023

miscco commented Sep 13, 2023

jecs commented Sep 13, 2023

gevtushenko commented Sep 13, 2023

gevtushenko left a comment

jecs commented Sep 14, 2023

gevtushenko commented Sep 19, 2023

gevtushenko commented Sep 19, 2023

jecs commented Sep 21, 2023

gevtushenko commented Sep 22, 2023

jecs commented Sep 27, 2023 •

edited

Loading

gevtushenko commented Sep 27, 2023

64-bit indexing for DeviceSegmentedReduce #414

64-bit indexing for DeviceSegmentedReduce #414

Conversation

jecs commented Sep 7, 2023 • edited by gevtushenko Loading

Description

Checklist

copy-pr-bot bot commented Sep 7, 2023

jecs commented Sep 7, 2023 • edited Loading

gevtushenko left a comment

Choose a reason for hiding this comment

jecs commented Sep 8, 2023 • edited Loading

miscco left a comment

Choose a reason for hiding this comment

jecs commented Sep 11, 2023

miscco commented Sep 13, 2023

jecs commented Sep 13, 2023

miscco commented Sep 13, 2023

jecs commented Sep 13, 2023

gevtushenko commented Sep 13, 2023

gevtushenko left a comment

Choose a reason for hiding this comment

jecs commented Sep 14, 2023

gevtushenko commented Sep 19, 2023

gevtushenko commented Sep 19, 2023

jecs commented Sep 21, 2023

gevtushenko commented Sep 22, 2023

jecs commented Sep 27, 2023 • edited Loading

gevtushenko commented Sep 27, 2023

jecs commented Sep 7, 2023 •

edited by gevtushenko

Loading

jecs commented Sep 7, 2023 •

edited

Loading

jecs commented Sep 8, 2023 •

edited

Loading

jecs commented Sep 27, 2023 •

edited

Loading