CUDA Reductions: Fix data races reported by Nvidia compute-sanitizer

[PhilMiller]

Detected with Nvidia's compute-sanitizer racecheck tool, with the tiny test case

#include <Kokkos_Core.hpp>
#include <cstdlib>
#include <iostream>

struct Sum {
  using value_type = double;

  KOKKOS_INLINE_FUNCTION
  void operator()(const int i, double& lsum) const {
    lsum += i;
  }
};

int main(int argc, char** argv)
{
  Kokkos::initialize(argc, argv);

  int n = std::atoi(argv[1]);
  std::cout << "Running with n = " << n << std::endl;
  {
    Kokkos::View<double*> v("foo", n);
    double val;
    Sum sum;
    Kokkos::parallel_reduce(n, sum, val);
    Kokkos::fence();
  }
  Kokkos::finalize();
  return 0;
}

Before this change:

$ compute-sanitizer --tool=racecheck ./redtest 1
========= COMPUTE-SANITIZER
Running with n = 1
========= WARNING: Race reported between Read access at 0xf10 in /scratch/pbmille/kokkos/install/cuda10/include/impl/Kokkos_FunctorAdapter.hpp:1571:Kokkos::Impl::CudaReductionsFunctor<Sum,void,bool=0,bool=0>::scalar_intra_warp_reduction(Sum const &,double*,bool,int)
=========     and Write access at 0x8a0 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_ReduceScan.hpp:557:Kokkos::Impl::CudaReductionsFunctor<Sum,void,bool=0,bool=0>::scalar_intra_block_reduction(Sum const &,double,bool,double*,int,double*) [3968 hazards]
========= 
========= WARNING: Race reported between Read access at 0x11a0 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_ReduceScan.hpp:548:Kokkos::Impl::CudaReductionsFunctor<Sum,void,bool=0,bool=0>::scalar_intra_warp_reduction(Sum const &,double*,bool,int)
=========     and Write access at 0x11d0 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_ReduceScan.hpp:548:Kokkos::Impl::CudaReductionsFunctor<Sum,void,bool=0,bool=0>::scalar_intra_warp_reduction(Sum const &,double*,bool,int) [144 hazards]
========= 
========= WARNING: Race reported between Write access at 0x1180 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_ReduceScan.hpp:572:Kokkos::Impl::CudaReductionsFunctor<Sum,void,bool=0,bool=0>::scalar_intra_block_reduction(Sum const &,double,bool,double*,int,double*)
=========     and Read access at 0x2d10 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_Parallel_Range.hpp:321:_ZNK6Kokkos4Impl14ParallelReduceI3SumNS_11RangePolicyIJNS_4CudaEEEENS_11InvalidTypeES4_EclEv [4 hazards]
========= 
========= RACECHECK SUMMARY: 3 hazards displayed (0 errors, 3 warnings)

After this change, the first two reports are eliminated:

$ compute-sanitizer --tool=racecheck --show-backtrace=yes ./redtest 1
========= COMPUTE-SANITIZER
Running with n = 1
========= WARNING: Race reported between Write access at 0x1180 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_ReduceScan.hpp:577:Kokkos::Impl::CudaReductionsFunctor<Sum,void,bool=0,bool=0>::scalar_intra_block_reduction(Sum const &,double,bool,double*,int,double*)
=========     and Read access at 0x2d10 in /scratch/pbmille/kokkos/install/cuda10/include/Cuda/Kokkos_Cuda_Parallel_Range.hpp:321:_ZNK6Kokkos4Impl14ParallelReduceI3SumNS_11RangePolicyIJNS_4CudaEEEENS_11InvalidTypeES4_EclEv [4 hazards]
========= 
========= RACECHECK SUMMARY: 1 hazard displayed (0 errors, 1 warning)

[dalg24]

Did you look at performance?

[PhilMiller]

Did you look at performance?

I haven't in the least.

For the first error and commit, @crtrott agreed that it's a legitimate defect in the code.

We haven't looked closely at the second commit fixing the second error yet.

I have a change that covers the third error, but I'm much less confident in it as yet

[crtrott]

I wonder if this needs to be

if ((lane_id + delta < 32) && (lane_id%(delta*2)==0)) {

[PhilMiller]

As I noted in chat, this suggestion didn't seem to make any difference to test results, either following the other changes in this PR, or with this PR plus my volatile wrappers. In the former case, everything still passed, in the latter, the half_t reduction unit test still failed.

[PhilMiller]

I'm not clear on what this change in condition is supposed to achieve

[crtrott]

basically in the first iteration you have this:

thread read_pos write_pos
0 1 0
1 2 1
2 3 2

so threads read something which gets overwritten by the next guy in the same line.

[ndellingwood]

@dalg24 @crtrott should this be cherry-picked to release-candidate-3.6.0?

[dalg24]

That would be good on second thought no let's not do that at the last minute.

[PhilMiller]

I would vote against cherry-picking it too. It's not been observed to cause errors in existing code otherwise, and it risks performance impacts.

[PhilMiller]

If it turns out to be entirely sound and low-impact, it could be part of a 3.6.1, which would support people making more use of Nvidia's compute-sanitizer