fix incorrect offset in CUDA/HIP parallel scan for < 4 byte types #5555

nmm0 · 2022-10-13T19:31:45Z

…WithTotal

nmm0 · 2022-10-13T19:33:25Z

@cwsmith this should solve the issue though there is additional work to be done in ensuring the correct type is used for the internal parallel scan when using `Kokkos::Experimental::inclusive_scan"

nmm0 · 2022-10-13T19:40:00Z

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp

@@ -1026,7 +1061,8 @@ class ParallelScanWithTotal<FunctorType, Kokkos::RangePolicy<Traits...>,
        if (!m_result_ptr_device_accessible)
          DeepCopy<HostSpace, CudaSpace, Cuda>(
              m_policy.space(), m_result_ptr,
-              m_scratch_space + (grid_x - 1) * size / sizeof(int), size);


I don't actually know where int came from in the original, but sizeof(word_size_type) is important here otherwise the offset of the result in the shared memory buffer will be incorrect (i.e. 0)

PhilMiller

Does any corresponding change need to be done for HIP?

nmm0 · 2022-10-13T19:48:50Z

Does any corresponding change need to be done for HIP?

From looking at the code it certainly appears so -- in fact the logic bug (size < HIP::size_type) appears to also be present in HIP's reduce

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp

masterleinad · 2022-10-13T19:53:14Z

I think we should make the same changes for HIP.

nmm0 · 2022-10-13T20:03:33Z

I think we should make the same changes for HIP.

Filed #5556

crtrott · 2022-10-14T15:04:32Z

Yeah lets get the HIP thing fixed here too, since the test doesn't pass anyway.

dalg24

Please beef up the description of the PR and relate it to #4156

dalg24 · 2022-10-17T14:03:49Z

core/unit_test/TestScan.hpp

+      // We should have a nice count from 0 to 1...
+      if (update != static_cast<value_type>(i)) {
+        int fail = errors()++;
+
+        // Limit the amount of output
+        if (fail < 20) {
+          KOKKOS_IMPL_DO_NOT_USE_PRINTF(
+              "TestSmallSizeTypeScan(%d) = %ld != %ld\n", i,
+              static_cast<long>(update), static_cast<long>(i));
+        }
+      }


Do not mix computation and verification. Launch another kernel for that.

Similarly, should I remove from the existing test?

core/unit_test/TestScan.hpp

Rombur · 2022-10-21T19:51:42Z

I have a patch that fixes the problem with HIP. How should we proceed?

dalg24 · 2022-10-21T20:12:21Z

Please go ahead and push to this PR

core/src/HIP/Kokkos_HIP_ReduceScan.hpp

core/unit_test/TestScan.hpp

masterleinad · 2022-10-24T12:37:08Z

core/unit_test/TestScan.hpp

+  KOKKOS_INLINE_FUNCTION
+  void init(value_type& update) const { update = 0; }
+


Suggested change

KOKKOS_INLINE_FUNCTION

void init(value_type& update) const { update = 0; }

core/unit_test/TestScan.hpp

…void overflow

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp

core/unit_test/TestScan.hpp

dalg24 · 2022-10-31T12:53:07Z

The HPX build is now passing. On the ORNL Jenkins CI server, there is one (likely unrelated) CUDA timing-based failure which I am willing to ignore.
We need one more approval though.

…ecializations Applied review suggestion to reduce the diff

…ition of word_size_type

masterleinad

Still looks good to me.

ldh4

Looks good to me. Just nitpicking on one thing.

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp

…#5555) * kokkos#5545: tests: add reproducer test for kokkos#5545 * kokkos#5545: cuda: use word_size_type for ParallelScan * kokkos#5545: tests: add reproducer for offset issue with ParallelScanWithTotal * kokkos#5545: cuda: add word_size_type for ParallelScanWithTotal * fix formatting * cuda: fix comments for word_size_type * tests: fix mismatched types in Scan test assert * Fix HIP parallel_scan when using types < 4 bytes * Fix indentation * Replace class with typename * tests: re-use old scan test for small types; add extra parameter to avoid overflow * tests: remove fence and change ImbalanceSz to value_type * Fix format * Re-introduce size_type member type in ParallelScan[WithTotal] CUDA specializations Applied review suggestion to reduce the diff * Nitpick in HIP too prefere size_type alias to HIP::size_type in definition of word_size_type * Apply Dongs suggestion and revert one more change I missed Co-authored-by: Bruno Turcksin <bruno.turcksin@gmail.com> Co-authored-by: Damien L-G <dalg24+github@gmail.com> Co-authored-by: Damien L-G <dalg24@gmail.com>

#5607) * fix incorrect offset in cuda parallel scan for < 4 byte types (#5555) * #5545: tests: add reproducer test for #5545 * #5545: cuda: use word_size_type for ParallelScan * #5545: tests: add reproducer for offset issue with ParallelScanWithTotal * #5545: cuda: add word_size_type for ParallelScanWithTotal * fix formatting * cuda: fix comments for word_size_type * tests: fix mismatched types in Scan test assert * Fix HIP parallel_scan when using types < 4 bytes * Fix indentation * Replace class with typename * tests: re-use old scan test for small types; add extra parameter to avoid overflow * tests: remove fence and change ImbalanceSz to value_type * Fix format * Re-introduce size_type member type in ParallelScan[WithTotal] CUDA specializations Applied review suggestion to reduce the diff * Nitpick in HIP too prefere size_type alias to HIP::size_type in definition of word_size_type * Apply Dongs suggestion and revert one more change I missed Co-authored-by: Bruno Turcksin <bruno.turcksin@gmail.com> Co-authored-by: Damien L-G <dalg24+github@gmail.com> Co-authored-by: Damien L-G <dalg24@gmail.com> * Remove stray comment for format accident * Fix bug spotted by Nic * cuda: fix missing value_type * HIP: fix missing value_type alias from cherry-pick Co-authored-by: Nicolas Morales <nmmoral@sandia.gov> Co-authored-by: Bruno Turcksin <bruno.turcksin@gmail.com>

…#5555) * kokkos#5545: tests: add reproducer test for kokkos#5545 * kokkos#5545: cuda: use word_size_type for ParallelScan * kokkos#5545: tests: add reproducer for offset issue with ParallelScanWithTotal * kokkos#5545: cuda: add word_size_type for ParallelScanWithTotal * fix formatting * cuda: fix comments for word_size_type * tests: fix mismatched types in Scan test assert * Fix HIP parallel_scan when using types < 4 bytes * Fix indentation * Replace class with typename * tests: re-use old scan test for small types; add extra parameter to avoid overflow * tests: remove fence and change ImbalanceSz to value_type * Fix format * Re-introduce size_type member type in ParallelScan[WithTotal] CUDA specializations Applied review suggestion to reduce the diff * Nitpick in HIP too prefere size_type alias to HIP::size_type in definition of word_size_type * Apply Dongs suggestion and revert one more change I missed Co-authored-by: Bruno Turcksin <bruno.turcksin@gmail.com> Co-authored-by: Damien L-G <dalg24+github@gmail.com> Co-authored-by: Damien L-G <dalg24@gmail.com>

nmm0 added 4 commits October 13, 2022 11:20

kokkos#5545: tests: add reproducer test for kokkos#5545

3795d4b

kokkos#5545: cuda: use word_size_type for ParallelScan

0340fae

kokkos#5545: tests: add reproducer for offset issue with ParallelScan…

74c0434

…WithTotal

kokkos#5545: cuda: add word_size_type for ParallelScanWithTotal

6133a6a

nmm0 requested a review from masterleinad October 13, 2022 19:31

nmm0 requested a review from crtrott October 13, 2022 19:39

nmm0 commented Oct 13, 2022

View reviewed changes

PhilMiller reviewed Oct 13, 2022

View reviewed changes

fix formatting

0c863a9

masterleinad added Blocks Promotion Overview issue for release-blocking bugs Patch Release labels Oct 13, 2022

masterleinad reviewed Oct 13, 2022

View reviewed changes

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp Outdated Show resolved Hide resolved

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp Outdated Show resolved Hide resolved

nmm0 added 2 commits October 13, 2022 13:09

cuda: fix comments for word_size_type

fd6c2d3

tests: fix mismatched types in Scan test assert

be7a065

nmm0 mentioned this pull request Oct 13, 2022

HIP Parallel Scan and Reduce have incorrect offset for types smaller than size_type #5556

Closed

dalg24 requested changes Oct 17, 2022

View reviewed changes

ajpowelsnl mentioned this pull request Oct 17, 2022

CHANGELOG 3.7.01 #5551

Closed

Rombur added 2 commits October 21, 2022 16:17

Fix HIP parallel_scan when using types < 4 bytes

593000e

Fix indentation

363cce2

dalg24 reviewed Oct 21, 2022

View reviewed changes

core/src/HIP/Kokkos_HIP_ReduceScan.hpp Outdated Show resolved Hide resolved

masterleinad reviewed Oct 24, 2022

View reviewed changes

PhilMiller mentioned this pull request Oct 25, 2022

#5333: CUDA: Use scratch space appropriate to small reduction elements in Team reductions #5334

Merged

Replace class with typename

f84becb

tests: re-use old scan test for small types; add extra parameter to a…

6581cb6

…void overflow

dalg24 approved these changes Oct 28, 2022

View reviewed changes

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp Show resolved Hide resolved

core/unit_test/TestScan.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Oct 28, 2022

View reviewed changes

core/unit_test/TestScan.hpp Outdated Show resolved Hide resolved

tests: remove fence and change ImbalanceSz to value_type

c1e2099

dalg24 reviewed Oct 28, 2022

View reviewed changes

core/unit_test/TestScan.hpp Outdated Show resolved Hide resolved

Fix format

4165288

masterleinad approved these changes Oct 31, 2022

View reviewed changes

dalg24 added 2 commits October 31, 2022 09:24

Re-introduce size_type member type in ParallelScan[WithTotal] CUDA sp…

cb2c1be

…ecializations Applied review suggestion to reduce the diff

Nitpick in HIP too prefere size_type alias to HIP::size_type in defin…

1cf94af

…ition of word_size_type

masterleinad approved these changes Oct 31, 2022

View reviewed changes

ldh4 approved these changes Oct 31, 2022

View reviewed changes

core/src/Cuda/Kokkos_Cuda_Parallel_Range.hpp Outdated Show resolved Hide resolved

Apply Dongs suggestion and revert one more change I missed

70c273c

dalg24 merged commit 3c842fb into kokkos:develop Oct 31, 2022

This was referenced Oct 31, 2022

[3.7.01] fix incorrect offset in cuda parallel scan for < 4 byte types #5607

Merged

CHANGELOG: 4.0 #5439

Closed

cwsmith mentioned this pull request Nov 2, 2022

inclusive_scan: std::int8_t input view and std::int32_t output view with CUDA backend produces incorrect result #5513

Closed

PhilMiller changed the title ~~fix incorrect offset in cuda parallel scan for < 4 byte types~~ fix incorrect offset in CUDA/HIP parallel scan for < 4 byte types Jan 25, 2023

PhilMiller mentioned this pull request Feb 6, 2023

Cuda: issues with custom reductions on arrays of half_t #5860

Closed

PhilMiller mentioned this pull request Jul 13, 2023

[HIP] Optimize parallel_reduce #6229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix incorrect offset in CUDA/HIP parallel scan for < 4 byte types #5555

fix incorrect offset in CUDA/HIP parallel scan for < 4 byte types #5555

nmm0 commented Oct 13, 2022

nmm0 commented Oct 13, 2022

nmm0 Oct 13, 2022 •

edited

PhilMiller left a comment

nmm0 commented Oct 13, 2022

masterleinad commented Oct 13, 2022

nmm0 commented Oct 13, 2022

crtrott commented Oct 14, 2022

dalg24 left a comment

dalg24 Oct 17, 2022

nmm0 Oct 17, 2022

Rombur commented Oct 21, 2022

dalg24 commented Oct 21, 2022

masterleinad Oct 24, 2022

dalg24 commented Oct 31, 2022

masterleinad left a comment

ldh4 left a comment

		KOKKOS_INLINE_FUNCTION
		void init(value_type& update) const { update = 0; }

fix incorrect offset in CUDA/HIP parallel scan for < 4 byte types #5555

fix incorrect offset in CUDA/HIP parallel scan for < 4 byte types #5555

Conversation

nmm0 commented Oct 13, 2022

nmm0 commented Oct 13, 2022

nmm0 Oct 13, 2022 • edited

Choose a reason for hiding this comment

PhilMiller left a comment

Choose a reason for hiding this comment

nmm0 commented Oct 13, 2022

masterleinad commented Oct 13, 2022

nmm0 commented Oct 13, 2022

crtrott commented Oct 14, 2022

dalg24 left a comment

Choose a reason for hiding this comment

dalg24 Oct 17, 2022

Choose a reason for hiding this comment

nmm0 Oct 17, 2022

Choose a reason for hiding this comment

Rombur commented Oct 21, 2022

dalg24 commented Oct 21, 2022

masterleinad Oct 24, 2022

Choose a reason for hiding this comment

dalg24 commented Oct 31, 2022

masterleinad left a comment

Choose a reason for hiding this comment

ldh4 left a comment

Choose a reason for hiding this comment

nmm0 Oct 13, 2022 •

edited