ginkgo icon indicating copy to clipboard operation
ginkgo copied to clipboard

Public common kernels

Open upsj opened this issue 3 years ago • 4 comments

This PR makes the simple run_kernel interface public and adds an example for how to use to allow portable kernel execution.

upsj avatar Dec 10 '21 18:12 upsj

format-rebase!

upsj avatar Dec 10 '21 18:12 upsj

Formatting rebase introduced changes, see Artifacts here to review them

ginkgo-bot avatar Dec 10 '21 18:12 ginkgo-bot

Error: The following files need to be formatted:

include/ginkgo/ginkgo.hpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

ginkgo-bot avatar Dec 11 '21 12:12 ginkgo-bot

@upsj Could you also point out which part you need the feedback? like the public part or header structure?

yhmtsai avatar Jan 12 '22 09:01 yhmtsai

Greeting from the preCICE team! We have some feedback regarding the implementation of this feature:

unified_kernels.cmake isn't installed, so this requires the sources to be present and basically turns off this feature for packages (incl. spack).

Calling this multiple times per target leads to an error due to the repeated target names. I suggest to either allow for a SUFFIX argument to avoid collisions, or to append sources to library targets that already exist using target_sources.

Setting properties on the generated targets isn't straight forward as the names their target names aren't communicated back to the caller. I suggest to either add an OUTPUT argument that returns a list of generated targets or define an interface library which is all generated targets link against. This generated target could either be fixed, such as ${targetname}_GinkgoDeps, or passed in using an option DEPTARGET mydeptarger.

Using the OUTPUT option could look like this:

find_package(Gingko REQUIRED)
ginkgo_add_unified_kernels(preciceCore _ginkgo_kernels
    src/mapping/ginkgo_kernels/GinkgoRBFKernels.cpp)
foreach(kernel in LISTS _ginkgo_kernels)
    target_include_directories($kernel PRIVATE src)
endforeach()

With a dependency graph like this:

flowchart LR;
preciceCore --> preciceCore_cuda --> |includes| inc(["src/"]);
preciceCore --> preciceCore_hip --> |includes| inc(["src/"]);
preciceCore --> preciceCore_omp --> |includes| inc(["src/"]);

The target option could look like this:

find_package(Gingko REQUIRED)
ginkgo_add_unified_kernels(preciceCore
    src/mapping/ginkgo_kernels/GinkgoRBFKernels.cpp)

target_include_directories(preciceCore_GinkgoDeps PRIVATE src)

With a dependency graph like this:

flowchart LR;
preciceCore --> preciceCore_cuda --> preciceCore_GinkgoDeps;
preciceCore --> preciceCore_hip --> preciceCore_GinkgoDeps;
preciceCore --> preciceCore_omp --> preciceCore_GinkgoDeps;
preciceCore_GinkgoDeps .-> |includes| inc(["src/"])

If you like to polish this up, I recommend using cmake_parse_arguments to create a nice interface:

ginkgo_add_unified_kernels(
   TARGET preciceCore
   RESULTS _ginkgo_kernels
   src/mapping/ginkgo_kernels/GinkgoRBFKernels.cpp
)

fsimonis avatar Feb 24 '23 14:02 fsimonis

This will not move forward in the near future, and would require a lot of work to update, so I'll close it and reopen a new one if it turns out we want this after all.

upsj avatar Nov 21 '23 12:11 upsj