SYCL: Prepare Parallel* for Graphs

[masterleinad]

Prerequisite for #6912. The Graphs implementation forces us to make the parallel construct implementations to be copyable. This is what this pull request is doing. In particular,

• we move all allocations into the execute member functions so that copied objects would not use the same memory
• move the scratch buffer locks to execute since they are not copyable.

Furthermore, the parallel_reduce TeamPolicy implementation was missing to lock the execution space instance' s m_mutexScratchSpace mutex.

[masterleinad]

The SYCL build is passing

[dalg24]

Please briefing explain where the copy is needed in the graph implementation and why we did not run into this issue with CUDA/HIP.
Also did you consider alternative approaches?

[masterleinad]

Please briefing explain where the copy is needed in the graph implementation and why we did not run into this issue with CUDA/HIP.

The errors look somewhat like

In file included from /home/darndt/kokkos_new/core/unit_test/sycl/TestSYCL_Graph.cpp:18:
In file included from /home/darndt/kokkos_new/core/unit_test/TestGraph.hpp:18:
In file included from /home/darndt/kokkos_new/core/src/Kokkos_Graph.hpp:161:
/home/darndt/kokkos_new/core/src/impl/Kokkos_GraphNodeImpl.hpp:147:21: error: call to implicitly-deleted copy constructor of 'Kokkos::Impl::GraphNodeKernelImpl<Kokkos::Experimental::SYCL, Kokkos::RangePolicy<Kokkos::Experimental::SYCL, Kokkos::Impl::IsGraphKernelTag>, Test::CountTestFunctor<Kokkos::Experimental::SYCL>, Kokkos::ParallelForTag>'
  147 |       : base_t(ex), m_kernel((KernelDeduced &&) arg_kernel) {}
      |                     ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/darndt/kokkos_new/core/src/impl/Kokkos_GraphNodeImpl.hpp:243:9: note: in instantiation of function template specialization 'Kokkos::Impl::GraphNodeImpl<Kokkos::Experimental::SYCL, Kokkos::Impl::GraphNodeKernelImpl<Kokkos::Experimental::SYCL, Kokkos::RangePolicy<Kokkos::Experimental::SYCL, Kokkos::Impl::IsGraphKernelTag>, Test::CountTestFunctor<Kokkos::Experimental::SYCL>, Kokkos::ParallelForTag>, Kokkos::Experimental::TypeErasedTag>::GraphNodeImpl<Kokkos::Impl::GraphNodeKernelImpl<Kokkos::Experimental::SYCL, Kokkos::RangePolicy<Kokkos::Experimental::SYCL, Kokkos::Impl::IsGraphKernelTag>, Test::CountTestFunctor<Kokkos::Experimental::SYCL>, Kokkos::ParallelForTag>>' requested here
  243 |       : base_t(ex, _graph_node_kernel_ctor_tag{},
[...]

The respective classes in Cuda and HIP are already copyable since we are using different approaches for guarding the scratch allocations.

Also did you consider alternative approaches?

I didn't want to try avoiding the copy in the Graphs implementation (which would likely be quite difficult) and moving the lock and scratch memory allocations to execute seemed like the best solution and consistent with Cuda/HIP.

[masterleinad]

Thanks for the suggestions!

[dalg24]

What's the deal with that lock?
Are you correcting an oversight/bug?

[masterleinad]

Yes, this looks like an oversight. We need both locks here so that we correctly deal with a TeamPolicy parallel_for or a (MD)RangePolicy parallel_reduce/parallel_scan submitted to the same execution space instance. I should have pointed this out in the pull request description.

Note that this is not necessary for the Graphs implementation but is a result of consistent refactoring. I want to deal with the acquire_team_scratch_space logic in a separate pull request later on (when allowing to discard SYCL events).

[dalg24]

Please update the description to mention that change.

[masterleinad]

Done.

[dalg24]

Ignoring

4: [  FAILED  ] 1 test, listed below:
4: [  FAILED  ] openmptarget.exec_space_thread_safety_range_scan
4: 
4:  1 FAILED TEST
 4/49 Test  #4: Kokkos_CoreUnitTest_OpenMPTarget ............***Failed  Error regular expression found in output. Regex=[  FAILED  ]177.40 sec

[ndellingwood]

@masterleinad does this merit a changelog entry for 4.4, or is this just internal implementation details for #6912 (which will get an entry)?

[dalg24]

Entry needed as there is a bug associated being resolved

[masterleinad]

@masterleinad does this merit a changelog entry for 4.4, or is this just internal implementation details for #6912 (which will get an entry)?

Added to #6914.