[hist] Use release-acquire in SnapshotAtomic

This ensures correctness on weakly-ordered systems. If not using
SnapshotAtomic, release stores would not be needed in FillAtomic.
However, tests indicate that it's not measurable in practice so
we use it unconditionally to keep the implementation simple.

Author: hahnjo

hist/histv7/benchmark/hist_benchmark_atomic.cxx

@@ -38,6 +38,15 @@ BENCHMARK_DEFINE_F(RHistAtomic_int, AtomicAdd)(benchmark::State &state)
 }
 BENCHMARK_REGISTER_F(RHistAtomic_int, AtomicAdd);
 
+BENCHMARK_DEFINE_F(RHistAtomic_int, AtomicAddRelease)(benchmark::State &state)
+{
+   for (auto _ : state) {
+      ROOT::Experimental::Internal::AtomicAddRelease(&fAtomic, 1);
+      benchmark::ClobberMemory();
+   }
+}
+BENCHMARK_REGISTER_F(RHistAtomic_int, AtomicAddRelease);
+
 BENCHMARK_DEFINE_F(RHistAtomic_int, AtomicLoad)(benchmark::State &state)
 {
    int load;
@@ -90,6 +99,15 @@ BENCHMARK_DEFINE_F(RHistAtomic_float, AtomicAdd)(benchmark::State &state)
 }
 BENCHMARK_REGISTER_F(RHistAtomic_float, AtomicAdd);
 
+BENCHMARK_DEFINE_F(RHistAtomic_float, AtomicAddRelease)(benchmark::State &state)
+{
+   for (auto _ : state) {
+      ROOT::Experimental::Internal::AtomicAddRelease(&fAtomic, 1.0f);
+      benchmark::ClobberMemory();
+   }
+}
+BENCHMARK_REGISTER_F(RHistAtomic_float, AtomicAddRelease);
+
 BENCHMARK_DEFINE_F(RHistAtomic_float, AtomicLoad)(benchmark::State &state)
 {
    float load;
@@ -142,6 +160,15 @@ BENCHMARK_DEFINE_F(RHistAtomic_double, AtomicAdd)(benchmark::State &state)
 }
 BENCHMARK_REGISTER_F(RHistAtomic_double, AtomicAdd);
 
+BENCHMARK_DEFINE_F(RHistAtomic_double, AtomicAddRelease)(benchmark::State &state)
+{
+   for (auto _ : state) {
+      ROOT::Experimental::Internal::AtomicAddRelease(&fAtomic, 1.0);
+      benchmark::ClobberMemory();
+   }
+}
+BENCHMARK_REGISTER_F(RHistAtomic_double, AtomicAddRelease);
+
 BENCHMARK_DEFINE_F(RHistAtomic_double, AtomicLoad)(benchmark::State &state)
 {
    double load;
@@ -185,14 +212,14 @@ BENCHMARK_DEFINE_F(RBinWithError, Inc)(benchmark::State &state)
 }
 BENCHMARK_REGISTER_F(RBinWithError, Inc);
 
-BENCHMARK_DEFINE_F(RBinWithError, AtomicInc)(benchmark::State &state)
+BENCHMARK_DEFINE_F(RBinWithError, AtomicIncRelease)(benchmark::State &state)
 {
    for (auto _ : state) {
-      fBin.AtomicInc();
+      fBin.AtomicIncRelease();
       benchmark::ClobberMemory();
    }
 }
-BENCHMARK_REGISTER_F(RBinWithError, AtomicInc);
+BENCHMARK_REGISTER_F(RBinWithError, AtomicIncRelease);
 
 BENCHMARK_DEFINE_F(RBinWithError, Add)(benchmark::State &state)
 {
@@ -203,14 +230,14 @@ BENCHMARK_DEFINE_F(RBinWithError, Add)(benchmark::State &state)
 }
 BENCHMARK_REGISTER_F(RBinWithError, Add);
 
-BENCHMARK_DEFINE_F(RBinWithError, AtomicAdd)(benchmark::State &state)
+BENCHMARK_DEFINE_F(RBinWithError, AtomicAddRelease)(benchmark::State &state)
 {
    for (auto _ : state) {
-      fBin.AtomicAdd(1.0);
+      fBin.AtomicAddRelease(1.0);
       benchmark::ClobberMemory();
    }
 }
-BENCHMARK_REGISTER_F(RBinWithError, AtomicAdd);
+BENCHMARK_REGISTER_F(RBinWithError, AtomicAddRelease);
 
 BENCHMARK_DEFINE_F(RBinWithError, AtomicLoad)(benchmark::State &state)
 {

hist/histv7/inc/ROOT/RBinWithError.hxx

@@ -97,9 +97,9 @@ private:
    }
 
 public:
-   void AtomicInc() { AtomicAdd(1.0, 1.0); }
+   void AtomicIncRelease() { AtomicAdd(1.0, 1.0); }
 
-   void AtomicAdd(double w) { AtomicAdd(w, w * w); }
+   void AtomicAddRelease(double w) { AtomicAdd(w, w * w); }
 
    /// Add another bin content using atomic instructions.
    ///

hist/histv7/inc/ROOT/RHistEngine.hxx

@@ -17,6 +17,7 @@
 #include "RWeight.hxx"
 
 #include <array>
+#include <atomic>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
@@ -500,7 +501,7 @@ public:
       RLinearizedIndex index = fAxes.ComputeGlobalIndexImpl<sizeof...(A)>(args);
       if (index.fValid) {
          assert(index.fIndex < fBinContents.size());
-         Internal::AtomicInc(&fBinContents[index.fIndex]);
+         Internal::AtomicIncRelease(&fBinContents[index.fIndex]);
       }
    }
 
@@ -524,7 +525,7 @@ public:
       RLinearizedIndex index = fAxes.ComputeGlobalIndexImpl<sizeof...(A)>(args);
       if (index.fValid) {
          assert(index.fIndex < fBinContents.size());
-         Internal::AtomicAdd(&fBinContents[index.fIndex], weight.fValue);
+         Internal::AtomicAddRelease(&fBinContents[index.fIndex], weight.fValue);
       }
    }
 
@@ -549,7 +550,7 @@ public:
       RLinearizedIndex index = fAxes.ComputeGlobalIndexImpl<sizeof...(A)>(args);
       if (index.fValid) {
          assert(index.fIndex < fBinContents.size());
-         Internal::AtomicAdd(&fBinContents[index.fIndex], weight);
+         Internal::AtomicAddRelease(&fBinContents[index.fIndex], weight);
       }
    }
 
@@ -573,7 +574,7 @@ public:
             RLinearizedIndex index = fAxes.ComputeGlobalIndexImpl<N>(t);
             if (index.fValid) {
                assert(index.fIndex < fBinContents.size());
-               Internal::AtomicAdd(&fBinContents[index.fIndex], weight.fValue);
+               Internal::AtomicAddRelease(&fBinContents[index.fIndex], weight.fValue);
             }
          } else {
             FillAtomic(t);
@@ -836,6 +837,10 @@ public:
       BinContentType tmp;
       bool changed;
       do {
+         // To guarantee correctness, we let the release operation(s) in FillAtomic synchronize with this acquire fence.
+         // This ensures that all previous writes become visible side-effects and the atomic loads will see them.
+         std::atomic_thread_fence(std::memory_order_acquire);
+
          changed = false;
          for (std::size_t i = 0; i < fBinContents.size(); i++) {
             Internal::AtomicLoad(&fBinContents[i], &tmp);

hist/histv7/inc/ROOT/RHistUtils.hxx

@@ -202,6 +202,18 @@ std::enable_if_t<std::is_arithmetic_v<T>, bool> AtomicCompareExchangeAcquire(T *
 #endif
 }
 
+template <typename T>
+std::enable_if_t<std::is_arithmetic_v<T>, bool> AtomicCompareExchangeRelease(T *ptr, T *expected, T *desired)
+{
+#ifndef _MSC_VER
+   return __atomic_compare_exchange(ptr, expected, desired, /*weak=*/false, __ATOMIC_RELEASE, __ATOMIC_RELAXED);
+#else
+   // Cannot specify the memory order directly, use an unconditional fence to avoid branching code.
+   std::atomic_thread_fence(std::memory_order_release);
+   return MSVC::AtomicOps<sizeof(T)>::CompareExchange(ptr, expected, desired);
+#endif
+}
+
 template <typename T>
 std::enable_if_t<std::is_arithmetic_v<T>> AtomicAddCompareExchangeLoop(T *ptr, T val)
 {
@@ -214,6 +226,18 @@ std::enable_if_t<std::is_arithmetic_v<T>> AtomicAddCompareExchangeLoop(T *ptr, T
    }
 }
 
+template <typename T>
+std::enable_if_t<std::is_arithmetic_v<T>> AtomicAddCompareExchangeReleaseLoop(T *ptr, T val)
+{
+   T expected;
+   AtomicLoad(ptr, &expected);
+   T desired = expected + val;
+   while (!AtomicCompareExchangeRelease(ptr, &expected, &desired)) {
+      // expected holds the new value; try again.
+      desired = expected + val;
+   }
+}
+
 #ifdef _MSC_VER
 namespace MSVC {
 inline void AtomicOps<8>::Add(void *ptr, const void *val)
@@ -243,17 +267,35 @@ std::enable_if_t<std::is_floating_point_v<T>> AtomicAdd(T *ptr, T val)
    AtomicAddCompareExchangeLoop(ptr, val);
 }
 
+template <typename T>
+std::enable_if_t<std::is_integral_v<T>> AtomicAddRelease(T *ptr, T val)
+{
+#ifndef _MSC_VER
+   __atomic_fetch_add(ptr, val, __ATOMIC_RELEASE);
+#else
+   // Cannot specify the memory order directly, use a fence.
+   std::atomic_thread_fence(std::memory_order_release);
+   MSVC::AtomicOps<sizeof(T)>::Add(ptr, &val);
+#endif
+}
+
+template <typename T>
+std::enable_if_t<std::is_floating_point_v<T>> AtomicAddRelease(T *ptr, T val)
+{
+   AtomicAddCompareExchangeReleaseLoop(ptr, val);
+}
+
 // For adding a double-precision weight to a single-precision bin content type, cast the argument once before the
 // compare-exchange loop.
-static inline void AtomicAdd(float *ptr, double val)
+static inline void AtomicAddRelease(float *ptr, double val)
 {
-   AtomicAdd(ptr, static_cast<float>(val));
+   AtomicAddRelease(ptr, static_cast<float>(val));
 }
 
 template <typename T>
-std::enable_if_t<std::is_arithmetic_v<T>> AtomicInc(T *ptr)
+std::enable_if_t<std::is_arithmetic_v<T>> AtomicIncRelease(T *ptr)
 {
-   AtomicAdd(ptr, static_cast<T>(1));
+   AtomicAddRelease(ptr, static_cast<T>(1));
 }
 
 template <typename T, typename U>
@@ -262,10 +304,16 @@ auto AtomicAdd(T *ptr, const U &add) -> decltype(ptr->AtomicAdd(add))
    return ptr->AtomicAdd(add);
 }
 
+template <typename T, typename U>
+auto AtomicAddRelease(T *ptr, const U &add) -> decltype(ptr->AtomicAddRelease(add))
+{
+   return ptr->AtomicAddRelease(add);
+}
+
 template <typename T>
-auto AtomicInc(T *ptr) -> decltype(ptr->AtomicInc())
+auto AtomicIncRelease(T *ptr) -> decltype(ptr->AtomicIncRelease())
 {
-   return ptr->AtomicInc();
+   return ptr->AtomicIncRelease();
 }
 
 } // namespace Internal

hist/histv7/test/hist_atomic.cxx

@@ -13,10 +13,10 @@ class RHistAtomic : public testing::Test {};
 using AtomicTypes = testing::Types<char, short, int, long, long long, float, double>;
 TYPED_TEST_SUITE(RHistAtomic, AtomicTypes);
 
-TYPED_TEST(RHistAtomic, AtomicInc)
+TYPED_TEST(RHistAtomic, AtomicIncRelease)
 {
    TypeParam a = 1;
-   ROOT::Experimental::Internal::AtomicInc(&a);
+   ROOT::Experimental::Internal::AtomicIncRelease(&a);
    EXPECT_EQ(a, 2);
 }
 
@@ -28,7 +28,15 @@ TYPED_TEST(RHistAtomic, AtomicAdd)
    EXPECT_EQ(a, 3);
 }
 
-// AtomicInc is implemented in terms of AtomicAdd, so it's sufficient to stress one of them.
+TYPED_TEST(RHistAtomic, AtomicAddRelease)
+{
+   TypeParam a = 1;
+   const TypeParam b = 2;
+   ROOT::Experimental::Internal::AtomicAddRelease(&a, b);
+   EXPECT_EQ(a, 3);
+}
+
+// AtomicInc* is implemented in terms of AtomicAdd*, so it's sufficient to stress one of them.
 TYPED_TEST(RHistAtomic, StressAtomicAdd)
 {
    static constexpr TypeParam Addend = 1;
@@ -47,25 +55,43 @@ TYPED_TEST(RHistAtomic, StressAtomicAdd)
    EXPECT_EQ(a, NAdds * Addend);
 }
 
-TEST(AtomicAdd, FloatDouble)
+TYPED_TEST(RHistAtomic, StressAtomicAddRelease)
+{
+   static constexpr TypeParam Addend = 1;
+   static constexpr std::size_t NThreads = 4;
+   // Reduce number of additions for char to avoid overflow.
+   static constexpr std::size_t NAddsPerThread = sizeof(TypeParam) == 1 ? 20 : 8000;
+   static constexpr std::size_t NAdds = NThreads * NAddsPerThread;
+
+   TypeParam a = 0;
+   StressInParallel(NThreads, [&] {
+      for (std::size_t i = 0; i < NAddsPerThread; i++) {
+         ROOT::Experimental::Internal::AtomicAddRelease(&a, Addend);
+      }
+   });
+
+   EXPECT_EQ(a, NAdds * Addend);
+}
+
+TEST(AtomicAddRelease, FloatDouble)
 {
    float a = 1;
    const double b = 2;
-   ROOT::Experimental::Internal::AtomicAdd(&a, b);
+   ROOT::Experimental::Internal::AtomicAddRelease(&a, b);
    EXPECT_EQ(a, 3);
 }
 
-TEST(RBinWithError, AtomicAdd)
+TEST(RBinWithError, AtomicAddRelease)
 {
    RBinWithError bin;
    bin.fSum = 1;
    bin.fSum2 = 2;
-   bin.AtomicAdd(1.5);
+   bin.AtomicAddRelease(1.5);
    EXPECT_EQ(bin.fSum, 2.5);
    EXPECT_EQ(bin.fSum2, 4.25);
 }
 
-TEST(RBinWithError, StressAtomicAdd)
+TEST(RBinWithError, StressAtomicAddRelease)
 {
    static constexpr double Addend = 2.0;
    static constexpr std::size_t NThreads = 4;
@@ -75,7 +101,7 @@ TEST(RBinWithError, StressAtomicAdd)
    RBinWithError bin;
    StressInParallel(NThreads, [&] {
       for (std::size_t i = 0; i < NAddsPerThread; i++) {
-         bin.AtomicAdd(Addend);
+         bin.AtomicAddRelease(Addend);
       }
    });
 
@@ -120,7 +146,7 @@ TEST(RBinWithError, StressAtomicLoad)
          }
       } else {
          for (std::size_t i = 0; i < NAddsPerThread; i++) {
-            bin.AtomicAdd(Addend);
+            bin.AtomicAddRelease(Addend);
          }
       }
    });

hist/histv7/test/hist_user.cxx

@@ -54,11 +54,11 @@ struct User {
       return *this;
    }
 
-   void AtomicInc() { ROOT::Experimental::Internal::AtomicInc(&fValue); }
+   void AtomicIncRelease() { ROOT::Experimental::Internal::AtomicIncRelease(&fValue); }
 
-   void AtomicAdd(double w) { ROOT::Experimental::Internal::AtomicAdd(&fValue, w); }
+   void AtomicAddRelease(double w) { ROOT::Experimental::Internal::AtomicAddRelease(&fValue, w); }
 
-   void AtomicAdd(const UserWeight &w) { ROOT::Experimental::Internal::AtomicAdd(&fValue, w.fWeight); }
+   void AtomicAddRelease(const UserWeight &w) { ROOT::Experimental::Internal::AtomicAddRelease(&fValue, w.fWeight); }
 
    void AtomicAdd(const User &rhs) { ROOT::Experimental::Internal::AtomicAdd(&fValue, rhs.fValue); }