lanl / ports-of-call

Performance Portability Utilities
https://lanl.github.io/ports-of-call/
BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Draft: Add portable query for exec space #33

Closed mauneyc-LANL closed 1 year ago

mauneyc-LANL commented 1 year ago

PR Summary

Adds portableExecIsHost() function that returns true if the execution space is on host, false if on device.

For PORTABILITY_STRATEGY_KOKKOS, compares Kokkos::DefaultExecutionSpace to Kokkos::HostSpace::execution_space. This can differentiate between openmp and cuda, for instance

For the explicit PORTABILITY_STRATEGY_*, just return true or false as expected.

This maybe useful e.g. https://github.com/lanl/spiner/pull/64

PR Checklist

mauneyc-LANL commented 1 year ago

I wasn't quite sure, save doing a lot of #ifdef KOKKOS_ENABLE_OPENMP ect. on the most correct way to implement a test. Ideas welcome

dholladay00 commented 1 year ago

@mauneyc-LANL I think using something similar to what I had is a bit more robust, for example does that do the right thing if OpenMP is the default memory space, I think this code below gets at the heart of the matter (do I need to copy memory somewhere else), or possibly we need this and other variables for different use cases.

     using HS = Kokkos::HostSpace;
     using DMS = Kokkos::DefaultExecutionSpace::memory_space;
     constexpr const bool execution_is_host{
         Kokkos::SpaceAccessibility<DMS, HS>::accessible};
mauneyc-LANL commented 1 year ago

@dholladay00 I've redone with your snippet.

If there are more flags like this that should be included, maybe best to close this PR and make a new one that's more comprehensive? Since this was originally spawned from an issue you had, you can probably judge how significant these changes are to get merged

mauneyc-LANL commented 1 year ago

@Yurlungur My initial commit was a function, but that was changed to a constant; I have no preference, though the constant should be namespaced - just let me know what namespace you want here.

The OpenMP specification has stuff to query the memory space, and Kokkos probably has some code committed to do this.

Yurlungur commented 1 year ago

@Yurlungur My initial commit was a function, but that was changed to a constant; I have no preference, though the constant should be namespaced - just let me know what namespace you want here.

Let's do PortsOfCall that's used elsewhere as well. It's inconsistent with the macros, but it feels more correct from a C++ style perspective.

The OpenMP specification has stuff to query the memory space, and Kokkos probably has some code committed to do this.

Ah good point. So for OpenMP Target since Kokkos knows at compile time, we should be able to query at compile time too.

chadmeyer commented 1 year ago

I agree with the sentiment of @Yurlungur that this should be compile time known so it should be a compile-time-evaluatable quantity -- whether a constexpr variable or constexpr function or whatnot. I'm not sure I know enough to weigh in as to which option is going to be better behaved.

mauneyc-LANL commented 1 year ago

@Yurlungur see 8204a93 for suggested changes

Yurlungur commented 1 year ago

LGTM thanks @mauneyc-LANL . One last request: Can you add one line to the docs mentioning this constant exists?

mauneyc-LANL commented 1 year ago

@Yurlungur see e82175b

dholladay00 commented 1 year ago

@mauneyc-LANL did you run the test with none and kokkos portability strategies?

mauneyc-LANL commented 1 year ago

Testing passes with kokkos

dholladay00 commented 1 year ago

I'm going to merge this in.