[HIP] Lock access to scratch memory when using Teams #3916
Conversation
| @@ -433,6 +433,9 @@ class ParallelFor<FunctorType, Kokkos::TeamPolicy<Properties...>, | |||
| int m_shmem_size; | |||
| void* m_scratch_ptr[2]; | |||
| int m_scratch_size[2]; | |||
| // Only let one ParallelFor/Reduce modify the team scratch memory. The | |||
| // constructor acquires the mutex which is released in the destructor. | |||
| std::unique_lock<std::mutex> m_scratch_lock; | |||
There was a problem hiding this comment.
std::lock_guard is non-copyable. This means you implicitly deleted the copy constructor and copy assignment. Was it intentional? Did you make sure it plays well with the kernel launching?
There was a problem hiding this comment.
This means you implicitly deleted the copy constructor and copy assignment.
True at the same time if you use the copy constructor currently, you are copying pointers without copying their data... Also I don't see what's your use case for that. Also I am using std::unique_lock instead of std::lock_guard. Unlike std::lock_guard, std::unique_lock is movable.
Did you make sure it plays well with the kernel launching?
I am not sure what you mean by that but all the tests pass on Tulip and on the CI.
There was a problem hiding this comment.
I missed it was std::unique_lock and not std::lock_guard. I can't remember how we pass the driver around in the kernel launching. This is not a trick question I was just curious if you considered it.
There is potentially a problem if multiple threads launch a
parallel_foror aparallel_reducekernel on the same stream and useTeams. Theparallel_forand theparallel_reducemay tried to reallocate the scratch memory and being used somewhere else. The current PR uses a mutex to ensure that only oneTeamparallel_fororparallel_reduceis running for a given instance. I am open to suggestion, if someone has a better solution.Note that CUDA has the same problem.