Fix atomic operations bug for Min and Max #6435

crtrott · 2023-09-12T17:59:50Z

This will fix atomic operations reported on slack, where atomic_min/max would give the wrong answer for a mix of negative/positive values. That was tracked down to a bug in the PTX for the atomic operations inside Desul, and not caught on our side due to test deficiencies.

dalg24

What about tpls/desul/include/desul/atomics/cuda/cuda_cc7_asm_atomic_op.inc_{forceglobal,generic}?

Also please confirm that we don't need to update the atomic_fetch_op headers

tpls/desul/include/desul/atomics/cuda/cuda_cc7_asm_atomic_op.inc_isglobal

tpls/desul/include/desul/atomics/cuda/cuda_cc7_asm_atomic_op.inc_predicate

masterleinad

Needs a corresponding desul pull request.

crtrott · 2023-09-14T02:03:24Z

desul/desul#109

crtrott · 2023-09-14T16:26:40Z

Updated atomic_fetch ops too.

The desul pull reqiuest has been merged.

masterleinad · 2023-09-21T16:26:38Z

There are still CI failures in the 32-bit build:

 RUN      ] openmp.atomic_operations_complexdouble
atomic_div failed with type N6Kokkos7complexIdEE
atomic_fetch_div failed with type N6Kokkos7complexIdEE
atomic_div_fetch failed with type N6Kokkos7complexIdEE
atomic_div_fetch did not return updated value with type N6Kokkos7complexIdEE
/__w/kokkos/kokkos/core/unit_test/TestAtomicOperations_complexdouble.hpp:40: Failure
Value of: (update != 0 ? atomic_op_test<DivAtomicTest, T, Kokkos::OpenMP>( old_val, update) : true)
  Actual: false
Expected: true
[  FAILED  ] openmp.atomic_operations_complexdouble (0 ms)
[ RUN      ] openmp.atomic_operations_complexfloat
atomic_div failed with type N6Kokkos7complexIfEE
atomic_fetch_div failed with type N6Kokkos7complexIfEE
atomic_div_fetch failed with type N6Kokkos7complexIfEE
atomic_div_fetch did not return updated value with type N6Kokkos7complexIfEE
/__w/kokkos/kokkos/core/unit_test/TestAtomicOperations_complexfloat.hpp:35: Failure
Value of: (update != 0 ? atomic_op_test<DivAtomicTest, T, Kokkos::OpenMP>( old_val, update) : true)
  Actual: false
Expected: true
[  FAILED  ] openmp.atomic_operations_complexfloat (0 ms)
[ RUN      ] openmp.atomic_operations_double
atomic_div failed with type d
atomic_fetch_div failed with type d
atomic_div_fetch failed with type d
atomic_div_fetch did not return updated value with type d
/__w/kokkos/kokkos/core/unit_test/TestAtomicOperations_double.hpp:25: Failure
Value of: (TestAtomicOperations::AtomicOperationsTestNonIntegralType< double, Kokkos::OpenMP>(i, end - i + start, t))
  Actual: false
Expected: true
[  FAILED  ] openmp.atomic_operations_double (0 ms)
[ RUN      ] openmp.atomic_operations_float
atomic_div failed with type d
atomic_fetch_div failed with type d
atomic_div_fetch failed with type d
atomic_div_fetch did not return updated value with type d
/__w/kokkos/kokkos/core/unit_test/TestAtomicOperations_float.hpp:25: Failure
Value of: (TestAtomicOperations::AtomicOperationsTestNonIntegralType< double, Kokkos::OpenMP>(i, end - i + start, t))
  Actual: false
Expected: true
[  FAILED  ] openmp.atomic_operations_float (0 ms)

We could ignore that failure by disabling the test for 32bit, adding a FIXME, and move on, of course. There are other tests failing for Kokkos::complex<double> in the 32-bit build.

masterleinad · 2023-09-26T14:04:16Z

core/unit_test/TestAtomicOperations_complexdouble.hpp

+    // disable division test for 32bit where we have accuracy issues with
+    // division atomics still compile it though


Suggested change

// disable division test for 32bit where we have accuracy issues with

// division atomics still compile it though

// FIXME_32BIT disable division test for 32bit where we have accuracy issues with

// division atomics, still compile it though

masterleinad · 2023-09-26T14:04:30Z

core/unit_test/TestAtomicOperations_complexfloat.hpp

-                                   old_val, update)
-                             : true));
+
+    // disable division test for 32bit where we have accuracy issues with


Suggested change

// disable division test for 32bit where we have accuracy issues with

// FIXME_32BIT disable division test for 32bit where we have accuracy issues with

masterleinad · 2023-09-26T14:04:40Z

core/unit_test/TestAtomicOperations_double.hpp

@@ -22,8 +22,11 @@ TEST(TEST_CATEGORY, atomic_operations_double) {
  const int end   = 11;
  for (int i = start; i < end; ++i) {
    for (int t = 0; t < 8; t++)
-      ASSERT_TRUE((TestAtomicOperations::AtomicOperationsTestNonIntegralType<
-                   double, TEST_EXECSPACE>(i, end - i + start, t)));
+      // disable division test for 32bit where we have accuracy issues with


Suggested change

// disable division test for 32bit where we have accuracy issues with

// FIXME_32BIT disable division test for 32bit where we have accuracy issues with

masterleinad · 2023-09-26T14:04:51Z

core/unit_test/TestAtomicOperations_float.hpp

@@ -22,8 +22,11 @@ TEST(TEST_CATEGORY, atomic_operations_float) {
  const int end   = 11;
  for (int i = start; i < end; ++i) {
    for (int t = 0; t < 8; t++)
-      ASSERT_TRUE((TestAtomicOperations::AtomicOperationsTestNonIntegralType<
-                   double, TEST_EXECSPACE>(i, end - i + start, t)));
+      // disable division test for 32bit where we have accuracy issues with


Suggested change

// disable division test for 32bit where we have accuracy issues with

// FIXME_32BIT disable division test for 32bit where we have accuracy issues with

masterleinad · 2023-09-26T14:05:55Z

Fine with me if you add some FIXME_32BIT comments.

Rombur · 2023-09-26T15:16:46Z

core/unit_test/TestAtomicOperations_complexdouble.hpp

+
+    // disable division test for 32bit where we have accuracy issues with
+    // division atomics still compile it though
+    if (sizeof(void*) == 8)


Kokkos/core/unit_test/TestAtomicOperations_complexdouble.hpp:43:8: error: suggest explicit braces to avoid ambiguous 'else' [-Werror=dangling-else] if (sizeof(void*) == 8) ^

ldh4

Mainly looked at changes in TestAtomicOperations files. Those look fine to me.

ldh4 · 2023-10-05T21:40:10Z

tpls/desul/include/desul/atomics/cuda/cuda_cc7_asm_atomic_fetch_op.inc_isglobal

@@ -205,4 +222,4 @@ __DESUL_IMPL_CUDA_ASM_ATOMIC_FETCH_BIN_OP()
 #undef __DESUL_IMPL_CUDA_ASM_ATOMIC_FETCH_INC
 #undef __DESUL_IMPL_CUDA_ASM_ATOMIC_FETCH_DEC
 #undef __DESUL_IMPL_CUDA_ASM_ATOMIC_FETCH_AND
-
+#undef __DESUL_IMPL_CUDA_ASM_ATOMIC_FETCH_BIN_OP


Why is this added? Was it just missing before?

crtrott added 2 commits September 8, 2023 10:03

Test atomic_op operations

58841a0

Improve Atomic Operations test

bf1c844

dalg24 reviewed Sep 12, 2023

View reviewed changes

masterleinad previously requested changes Sep 12, 2023

View reviewed changes

crtrott added 2 commits September 13, 2023 19:57

Fix desul CUDA atomic assembly

2119a54

Desul: delete unused files

10c47a5

crtrott force-pushed the fix-atomics branch from 5a2f48b to 10c47a5 Compare September 14, 2023 01:58

Fix warning

112eede

Disable atomic division test for floating point on 32 bit

2028753

masterleinad reviewed Sep 26, 2023

View reviewed changes

Rombur reviewed Sep 26, 2023

View reviewed changes

crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Oct 4, 2023

Add FIXME_32BIT and curly braces

0c57a70

masterleinad approved these changes Oct 4, 2023

View reviewed changes

ldh4 approved these changes Oct 5, 2023

View reviewed changes

crtrott merged commit 8181d70 into kokkos:develop Oct 6, 2023
27 of 28 checks passed

crtrott deleted the fix-atomics branch October 6, 2023 14:44

masterleinad mentioned this pull request Oct 11, 2023

CHANGELOG: 4.2.0 #6197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix atomic operations bug for Min and Max #6435

Fix atomic operations bug for Min and Max #6435

crtrott commented Sep 12, 2023

dalg24 left a comment

masterleinad left a comment

crtrott commented Sep 14, 2023

crtrott commented Sep 14, 2023

masterleinad commented Sep 21, 2023 •

edited

masterleinad Sep 26, 2023

masterleinad Sep 26, 2023

masterleinad Sep 26, 2023

masterleinad Sep 26, 2023

masterleinad commented Sep 26, 2023

Rombur Sep 26, 2023

ldh4 left a comment

ldh4 Oct 5, 2023

crtrott Oct 6, 2023

		// disable division test for 32bit where we have accuracy issues with
		// division atomics still compile it though

	// disable division test for 32bit where we have accuracy issues with
	// FIXME_32BIT disable division test for 32bit where we have accuracy issues with

Fix atomic operations bug for Min and Max #6435

Fix atomic operations bug for Min and Max #6435

Conversation

crtrott commented Sep 12, 2023

dalg24 left a comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

crtrott commented Sep 14, 2023

crtrott commented Sep 14, 2023

masterleinad commented Sep 21, 2023 • edited

masterleinad Sep 26, 2023

Choose a reason for hiding this comment

masterleinad Sep 26, 2023

Choose a reason for hiding this comment

masterleinad Sep 26, 2023

Choose a reason for hiding this comment

masterleinad Sep 26, 2023

Choose a reason for hiding this comment

masterleinad commented Sep 26, 2023

Rombur Sep 26, 2023

Choose a reason for hiding this comment

ldh4 left a comment

Choose a reason for hiding this comment

ldh4 Oct 5, 2023

Choose a reason for hiding this comment

crtrott Oct 6, 2023

Choose a reason for hiding this comment

masterleinad commented Sep 21, 2023 •

edited