Finalize deep_copy_space early avoiding printing to std::cerr for Cuda

[masterleinad]

We are currently printing an error message to std::cerr when finalizing Cuda execution spaces as the first commit shows.

The problem is that we are finalizing deep_copy_space since that needs to deallocate data which calls a fence on the default execution space and we check if the execution space has already unset m_dev at this point. Wd didn't see this so far, since we are only printing to std::cerr but not throwing an exception or aborting.
The proposed fix is to finalize deep_copy_space before unsettling m_dev.

While looking at this, I also found that some of the code in finalize is guarded by a check for scratch space which didn't make sense to me.

Throwing in finalize is debatable, of course, but makes the problem obvious (in the CI).

[dalg24]

In general, I would say looks good. (I would suggest we call abort() instead of throwing an exception though.)
But, I suppose you are looking into this because of @stanmoore1 report on Slack

Stan Moore [12:14 PM]
I'm still seeing this error in LAMMPS:

Kokkos::Cuda::Cuda instance constructor : ERROR device not initialized

and I don't quite follow how either of Cuda::Cuda() or Cuda::Cuda(cudaStream_t, bool) would get called.

Please elaborate.

[masterleinad]

The failing stacktrace looks like

abort@libc.so.6
Kokkos::Impl::CudaInternal::verify_is_initialized@../../lib/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:336
Kokkos::Cuda::Cuda@../../lib/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:880
Kokkos::Impl::HostInaccessibleSharedAllocationRecordCommon<Kokkos::CudaSpace>::get_record@../../lib/kokkos/core/src/impl/Kokkos_SharedAlloc_timpl.hpp:265
Kokkos::Impl::CudaInternal::finalize@../../lib/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:731
Kokkos::Impl::CudaInternal::finalize@../../lib/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:774
Kokkos::Cuda::impl_finalize@../../lib/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:875
Kokkos::Impl::CudaSpaceInitializer::finalize@../../lib/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:947
Kokkos::Impl::ExecSpaceManager::finalize_spaces@../../lib/kokkos/core/src/impl/Kokkos_Core.cpp:125
Kokkos::Impl::(anonymous@../../lib/kokkos/core/src/impl/Kokkos_Core.cpp:561
Kokkos::finalize@../../lib/kokkos/core/src/impl/Kokkos_Core.cpp:1033
LAMMPS_NS::KokkosLMP::finalize@../kokkos.cpp:350
main@../main.cpp:108

[dalg24]

Thanks so it is

kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp

Line 745 in acb3434

RecordCuda::decrement(RecordCuda::get_record(m_scratchFlags));

which calls

kokkos/core/src/impl/Kokkos_SharedAlloc_timpl.hpp

Lines 264 to 271 in acb3434

	if (alloc_ptr) {
	typename MemorySpace::execution_space exec_space;
	Kokkos::Impl::DeepCopy<HostSpace, MemorySpace, decltype(exec_space)>(
	exec_space, &head, head_cuda, sizeof(SharedAllocationHeader));
	exec_space.fence(
	"HostInaccessibleSharedAllocationRecordCommon::get_record(): fence "
	"after copying header to HostSpace");
	}

and the issue is our default constructing an execution space on L265.
This is a release 3.6 defect (introduced in #4478)

I suppose we should use Cuda::impl_static_fence(...) after the deep copy instead of fence().

[masterleinad]

I suppose we should use Cuda::impl_static_fence(...) after the deep copy instead of fence().

I still think we should just fence the stream we are using for the copy.

[crtrott]

Yeah lets abort.

[masterleinad]

Yeah lets abort.

See fbea785.