Check for overflow during backend initialization (Cuda, HIP, SYCL) #6159

cz4rs · 2023-05-24T13:41:08Z

fixes #3805

refactor the code to avoid potential overflow when possible
check for overflow when initializing scratch flags / scratch space
- note: the code will Kokkos::abort when an overflow is detected (which should be fine in this case since we cannot allocate that much memory anyways)

masterleinad

Please also fix the other backends that have the same code.

core/src/Cuda/Kokkos_Cuda_Instance.cpp

fnrizzi · 2023-05-24T15:45:34Z

please make the description more descriptive

masterleinad

Looks OK to me.

core/src/Cuda/Kokkos_Cuda_Instance.cpp

core/src/impl/Kokkos_Utilities.hpp

masterleinad · 2023-05-31T14:27:57Z

Please update the title and description to describe the full scope of this pull request.

cz4rs · 2023-05-31T15:20:49Z

Please update the title and description to describe the full scope of this pull request.

Done 👌

core/src/impl/Kokkos_Utilities.hpp

core/src/HIP/Kokkos_HIP_Instance.cpp

cz4rs · 2023-06-26T11:38:58Z

Looks like HIP failures are unrelated.

Rombur · 2023-06-27T14:21:32Z

Retest this please

core/unit_test/TestCheckedIntegerOps.hpp

- use correct namespace - only used overflow detection when necessary

core/src/SYCL/Kokkos_SYCL_Instance.cpp

Co-authored-by: Daniel Arndt <[email protected]>

Rombur · 2023-07-10T13:24:52Z

core/src/Cuda/Kokkos_Cuda_Instance.cpp

+    std::size_t alloc_size;
+    if (multiply_overflow(m_scratchFlagsCount, sizeScratchGrain, alloc_size)) {
+      Kokkos::abort("Arithmetic overflow detected.");
+    }


I don't find this code intuitive. What about we do sth like

std::size_t alloc_size = multiply_overflow(m_scratchFlagsCount, sizeScratchGrain);

and we put the Kokkos::abort inside multiply_overflow. The code is easier to read and we don't need to repeat the Kokkos::abort everywhere.

See ffe1120.

crtrott · 2023-07-26T22:38:52Z

core/src/impl/Kokkos_CheckedIntegerOps.hpp

+  } else {
+    return true;
+  }
+}


Small follow up for someone: there are builtins we could use here: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

cz4rs changed the base branch from master to develop May 24, 2023 13:41

cz4rs force-pushed the fix-overflow branch from 5fd4236 to a33c4ba Compare May 24, 2023 13:42

masterleinad requested changes May 24, 2023

View reviewed changes

core/src/Cuda/Kokkos_Cuda_Instance.cpp Outdated Show resolved Hide resolved

core/src/Cuda/Kokkos_Cuda_Instance.cpp Outdated Show resolved Hide resolved

cz4rs marked this pull request as draft May 24, 2023 19:36

cz4rs force-pushed the fix-overflow branch from 2d635ae to b05585c Compare May 25, 2023 09:01

cz4rs marked this pull request as ready for review May 25, 2023 11:41

cz4rs requested a review from masterleinad May 25, 2023 11:41

masterleinad approved these changes May 25, 2023

View reviewed changes

dalg24 reviewed May 25, 2023

View reviewed changes

core/src/Cuda/Kokkos_Cuda_Instance.cpp Outdated Show resolved Hide resolved

core/src/Cuda/Kokkos_Cuda_Instance.cpp Outdated Show resolved Hide resolved

cz4rs force-pushed the fix-overflow branch from 3c2cdde to 96cffa6 Compare May 26, 2023 19:06

masterleinad reviewed May 26, 2023

View reviewed changes

core/src/impl/Kokkos_Utilities.hpp Outdated Show resolved Hide resolved

cz4rs force-pushed the fix-overflow branch 2 times, most recently from 425f765 to 9267043 Compare May 27, 2023 13:29

cz4rs requested review from dalg24 and masterleinad May 27, 2023 16:28

cz4rs changed the title ~~Fix potential arithmetic overflow in Cuda backend~~ Check for overflow during backend initialization (Cuda, HIP, SYCL) May 31, 2023

dalg24 reviewed May 31, 2023

View reviewed changes

core/src/impl/Kokkos_Utilities.hpp Outdated Show resolved Hide resolved

dalg24 reviewed May 31, 2023

View reviewed changes

core/src/HIP/Kokkos_HIP_Instance.cpp Show resolved Hide resolved

cz4rs marked this pull request as draft May 31, 2023 19:29

cz4rs force-pushed the fix-overflow branch from 9267043 to 0f25787 Compare June 22, 2023 15:12

cz4rs marked this pull request as ready for review June 23, 2023 15:11

cz4rs force-pushed the fix-overflow branch from 8184ab0 to e9d6fbe Compare June 23, 2023 21:29

cz4rs requested a review from dalg24 June 26, 2023 11:38

dalg24 reviewed Jun 27, 2023

View reviewed changes

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

dalg24 reviewed Jun 27, 2023

View reviewed changes

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

cz4rs marked this pull request as draft June 29, 2023 11:52

cz4rs force-pushed the fix-overflow branch from 52ee567 to cc79077 Compare July 3, 2023 06:50

cz4rs added 8 commits July 5, 2023 16:39

Fix potential overflow

e0ce16d

Rewrite update condition to avoid overflow

b88d732

Avoid overflow when setting scratch flags

60d2f54

Restrict overflow check to unsigned integral types

6786c0f

Rework multiply_overflow

6c3e1a1

Use || instead of or (MSVC fix)

83afd6d

Fix SYCL build

3602793

- use correct namespace - only used overflow detection when necessary

Clean up unit tests

387d026

cz4rs force-pushed the fix-overflow branch from 81d3b67 to 42a6958 Compare July 5, 2023 14:47

Use correct variables and consistent formatting

5a6de56

cz4rs force-pushed the fix-overflow branch from 42a6958 to 5a6de56 Compare July 5, 2023 14:48

cz4rs marked this pull request as ready for review July 5, 2023 19:54

cz4rs requested review from dalg24 and masterleinad July 5, 2023 19:54

masterleinad reviewed Jul 5, 2023

View reviewed changes

core/src/SYCL/Kokkos_SYCL_Instance.cpp Outdated Show resolved Hide resolved

Add a FIXME for SYCL sizeScratchGrain

bfb8de5

Co-authored-by: Daniel Arndt <[email protected]>

masterleinad approved these changes Jul 7, 2023

View reviewed changes

Rombur reviewed Jul 10, 2023

View reviewed changes

Add multiply_overflow_abort helper

ffe1120

cz4rs requested a review from Rombur July 22, 2023 21:47

Rombur approved these changes Jul 24, 2023

View reviewed changes

crtrott reviewed Jul 26, 2023

View reviewed changes

crtrott approved these changes Jul 26, 2023

View reviewed changes

crtrott merged commit ced2451 into kokkos:develop Jul 26, 2023

cz4rs mentioned this pull request Jul 27, 2023

Use checked arithmetic builtins for overflow detection #6313

Merged

cz4rs mentioned this pull request Aug 4, 2023

arithmetic overflow potential #3805

Closed

dalg24 mentioned this pull request Sep 1, 2023

Fixup checked integer operations death test not properly setup #6400

Merged

fnrizzi mentioned this pull request Oct 3, 2023

CHANGELOG: 4.2.0 #6197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for overflow during backend initialization (Cuda, HIP, SYCL) #6159

Check for overflow during backend initialization (Cuda, HIP, SYCL) #6159

cz4rs commented May 24, 2023 •

edited

Loading

masterleinad left a comment

fnrizzi commented May 24, 2023

masterleinad left a comment

masterleinad commented May 31, 2023

cz4rs commented May 31, 2023

cz4rs commented Jun 26, 2023

Rombur commented Jun 27, 2023

Rombur Jul 10, 2023

cz4rs Jul 22, 2023

crtrott Jul 26, 2023

Check for overflow during backend initialization (Cuda, HIP, SYCL) #6159

Check for overflow during backend initialization (Cuda, HIP, SYCL) #6159

Conversation

cz4rs commented May 24, 2023 • edited Loading

masterleinad left a comment

Choose a reason for hiding this comment

fnrizzi commented May 24, 2023

masterleinad left a comment

Choose a reason for hiding this comment

masterleinad commented May 31, 2023

cz4rs commented May 31, 2023

cz4rs commented Jun 26, 2023

Rombur commented Jun 27, 2023

Rombur Jul 10, 2023

Choose a reason for hiding this comment

cz4rs Jul 22, 2023

Choose a reason for hiding this comment

crtrott Jul 26, 2023

Choose a reason for hiding this comment

cz4rs commented May 24, 2023 •

edited

Loading