Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary(?) check for host execution space initialization from Cuda initialization #2652

Closed
streichler opened this issue Jan 16, 2020 · 5 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting

Comments

@streichler
Copy link

The initialization of a CudaInternal in the Cuda execution space initialization includes a check that the default host execution space has been initialized:

#ifdef KOKKOS_ENABLE_DEPRECATED_CODE
if (!HostSpace::execution_space::is_initialized()) {
#else
if (!HostSpace::execution_space::impl_is_initialized()) {
#endif
const std::string msg(
"Cuda::initialize ERROR : HostSpace::execution_space is not "
"initialized");
throw_runtime_exception(msg);
}

This check is unnecessary if Kokkos is initialized via Kokkos::initialize because all the host execution spaces are done before any accelerator spaces, and it's problematic for Legion+Kokkos interop (#2651) because the thread doing Cuda initialization may not see that the host execution space (e.g. OpenMP) has been initialized on a different thread.

Any of the following approaches would resolve this from Legion's perspective:

  1. delete the check entirely if it indeed can never trigger through normal usage
  2. replace it with a more focused initialization of what it actually needs (which I think is just a call to Impl::SharedAllocationRecord<void, void>::tracking_enable()?)
  3. add an optional flag to Kokkos::Cuda::impl_initialize that requests that the test be omitted
  4. some other resolution suggested by the Kokkos team
@crtrott
Copy link
Member

crtrott commented Jan 23, 2020

I don't get this one: I believe we need the DefaultHostExecutionSpace to be initialized for some operations involving CUDA, such as creating a HostMirror of a view, which will create a View with Device<HostSpace::execution_space,HostSpace> and then run the initialization parallel_for for it. So if that execution_space doesn't work on this thread you are screwed or?

@streichler
Copy link
Author

I believe we need the DefaultHostExecutionSpace to be initialized for some operations involving CUDA, such as creating a HostMirror of a view

In the short term, that's not a problem because the current Kokkos+Legion interop is only using unmanaged views. Longer term, if the Cuda execution space really needed to use a host execution space for some work, we'd either need to have that work go to a separate host execution space instance so that it doesn't interfere with work that the application is sending to the "main" host execution space, or have Cuda use an execution space like Serial that doesn't care where it's launched from.

Can you describe for me a scenario where a user who is using the standard (i.e. non-impl) Kokkos entry points could ever see this error message?

@crtrott
Copy link
Member

crtrott commented Jan 23, 2020

No there is no scenario right now where this would happen. This can only come through calling the impl init thingies.

@streichler
Copy link
Author

Here is the change I'm successfully using in a fork: streichler@89fc1c1

Note that the skipping of the host exec space init needs to happen during both the initial cuda execution space init (based on an argument to the call) as well as at the creation of any cuda execution space instance (where the skipping can be unconditional because these always occur
after the space has been initialized).

@crtrott
Copy link
Member

crtrott commented Mar 3, 2020

Would for now a KOKKOS_IMPL_TURN_OFF_CUDA_HOST_INIT_CHECK macro you have to add to your CXXFLAGS for building Kokkos do the job? In particular as a testing vehicle?

@crtrott crtrott added the Enhancement Improve existing capability; will potentially require voting label Mar 3, 2020
@crtrott crtrott added this to To do in Milestone: Release 3.1 via automation Mar 3, 2020
@crtrott crtrott assigned crtrott and unassigned janciesko Mar 3, 2020
@crtrott crtrott added this to the Tentative 3.1 Release milestone Mar 3, 2020
@crtrott crtrott moved this from To do to Done in Milestone: Release 3.1 Mar 10, 2020
@crtrott crtrott closed this as completed Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
No open projects
Development

No branches or pull requests

3 participants