Basic Kokkos Extension

[MarcelKoch]

This PR adds a mapping to Kokkos types and functions to choose the Ginkgo executor based on the Kokkos execution space.

It also introduces the Ginkgo::ext and Ginkgo::ext::kokkos targets to make it easy for applications to opt into this functionality.

Currently, the mapping only supports array matrix::Dense and device_matrix_data. Other mappings are possible, but not necessary at the moment.

Old PR Description

Note: This description is not reflective of the changes anymore. They are left here, because I want to move them into their on PR at some point.

Instead of providing the kokkos type mapping directly, an extra layer of mapping classes is introduced. This extra layer provides mapping for 'complex' Ginkgo types (i.e. not gko::array or gko::matrix::Dense). These mappings can then be concretized for different frameworks such as Kokkos, or maybe Raja in the future, by specifying how gko::array and gko::matrix::Dense are mapped in these frameworks.

Currently, we only offer mapping types in the direction framework -> Ginkgo (through our array views), and this can make the other direction possible.

The mapping could also be used in Ginkgo internally for the common kernels, but that would require some changes to the kernel calls.

Todo:

• adding more type mappings?

Note:

This exposes some internal implementation detail. A key factor for this PR is to define (in the future) for nearly all of our types a basic struct that describes the memory layout in terms of 1D and 2D arrays. This would make interface conforming changes to the internal storage format more difficult. On the other hand, it would also allow us to define low-level representation of our classes, that could be passed into kernels.

[yhmtsai]

if you avoid the extensions header are included in the main header, the macro is not required.

[MarcelKoch]

I would rather leave it here, in case the headers are included individually.

[yhmtsai]

I thought it indeed can be included individually, doesn't it?
There's nothing rely on ginkgo from kokkos extensions if native_type.hpp is always included in ginkgo.hpp or is included from extensions.
User can link kokkos and ginkgo with including these two files such that they can use it.

[MarcelKoch]

Maybe I didn't get your point, but yes the headers could be included individually. That's why I left my first comment.

[yhmtsai]

I was thinking of the case
users do not configure ginkgo with Kokkos extension but they use these header in the end.
They definitely need to take care of Kokkos dependence.
This condition seems to be unnecessary?

[MarcelKoch]

You're right. If they don't have kokkos, then the include will fail, which should give enough of an error.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

[upsj]

Since this is a header-only extension, I agree with @yhmtsai that the header can be made unconditional. A consequence of this is that we can include Kokkos in kokkos_*.hpp headers and let users take care of finding the Kokkos package and linking against it, since they need it for their application as well.

That would significantly trim down the CMake changes we need to do (and also remove the need for a separate EXTENSION flag), limiting it to only examples and tests.

[MarcelKoch]

I don't think it should be unconditional. My reasoning is:

• We already decided to add part of the file config as an 'extension' to Ginkgo. So the cmake setup would be necessary anyway, and I think that the current approach adds a clear structure to it.
• It states the dependencies clearly. If the option GINKGO_EXTENSION_KOKKOS is enabled, Kokkos needs to be available. I would prefer that over the implicit dependency and hoping that the end user does the right thing.

Of course, I also dislike the hack to disable the CXX_COMPILER_LAUNCHER. I can remove it, but it will require some more code for the tests.

[upsj]

I already noted in #1389 that none of the additional CMake setup is actually necessary, because the only thing we are testing is a single function that is or is not compiled based on whether JSON support was activated.
My concern is that the amount of CMake configurations would grow quickly once we start adding header-only mappings for other popular interfaces (e.g. Eigen or C++20 mdspan), so the best thing to do is keep them as lightweight as possible.
With a header called kokkos_*, I believe we can trust users to only use it if they use Kokkos.

On the compiler launcher, IMO this is mainly a CI/development thing. In CI we can control it ourselves, for development, maybe it does make sense to provide CMake developer presets that set a lot of useful variables (CMAKE_EXPORT_COMPILE_COMMANDS, CMAKE_LINK_DEPENDS_NO_SHARED, ...) and leave ccache OFF by default?

[yhmtsai]

They are somehow different in my mind.
It makes the extension inside the ginkgo_main, but the parser of file config is completely out of ginkgo_main.

The parser of property_tree is not just one function. for example, it can also be used to hide the JSON package requirement from the application, which was mentioned long time ago. In this case, it needs to be compiled, but still out of the main ginkgo.
The extension header is not in the ginkgo main header and it does not contain condition.
Even if user does not compile it, they can still include it by their own (only the header-only part).

If we still go the direction putting Kokkos in the main header, it's only required to add the condition in main header but not in the header itself. The header (Kokkos) can still be used by user directly.

For me, the extension on top of ginkgo should be pluggable. papi in my mind might also be an extension not feature if it does not need to be inside the ginkgo. hwloc is still in ginkgo itself because it changes the memory behavior (in theory).

[MarcelKoch]

@yhmtsai The kokkos stuff is not part of ginkgo_main and will not be. I consider only ginkgo/core to be ginkgo_main.

[upsj]

We have a lot of optional things (MPI, PAPI, TAU, roctx, rocfft, hwloc) inside Ginkgo. The parser is just a single function to the user interface (which as I just noticed is also header-only, so it's almost exactly the same scenario as with Kokkos).
Everything that the users themselves need to enable in their code to interact with us (which applies to all aforementioned header-only extensions) does not need any build system interaction from our side except for tests/examples. We can save a lot of complexity in terms of building, packaging and general CI if we restrict the number of ways Ginkgo builds can differ. We already have quite a large combinatorial set of features, I think we'd do best if we avoided growing it further.

[pratikvn]

Most systems, alteast the HPC systems, have Kokkos installed. I am not sure always building with Kokkos is a good idea. While the explosion of options and the increase in build system complexity is definitely a concern, but we should also consider that always building with third-party options can unnecessarily increase the library object size.

[yhmtsai]

If you change the update_ginkgo_header.sh to exclude the ginkgo extension, then it is not part of ginkgo_main.
Otherwise, it is in ginkgo.hpp currently, right? In this case, it is part of ginkgo main.

#1389 mentioned it will move the parser to another pr.
It's still there just to show the interaction between json, so I would not like to push the complete set there.
It can be more than header only as I said if we would like to avoid the json dependency from the user application.

There are two things for the optional stuff:

1. the stuff changes the behavior and it will not be able to be implemented outside. (feature in ginkgo)
2. the stuff can be on top of ginkgo (ginkgo extension, no matter if it is header only or not)
   We can try to make the cmake less for the head-only extension.
   The extension embedded in the ginkgo itself is a bit weird to me.

In packaging, putting into extensions should be easier, right? If you want to provide pre-compiled library for user to switch these options, you need to provide all ginkgo compiled with these combinations but you can provide ginkgo + extension to produce these combinations easily.

[MarcelKoch]

Most systems, alteast the HPC systems, have Kokkos installed. I am not sure always building with Kokkos is a good idea.

I'm not sure who is building Kokkos here. We are not. It's only a cmake dependency if the corresponding option is set. But the package must already be installed.

[pratikvn]

No, I mean building Ginkgo with the Kokkos dependency always enabled.

[MarcelKoch]

By default it's disabled. Only if the user request it, it will be added.

[pratikvn]

Yes, that is what is now. But if I understand correctly, what @upsj is suggesting is that you remove the option to disable it, thereby always building Ginkgo with Kokkos (if Kokkos is available), which IMO is not a good idea.

[upsj]

@pratikvn I believe you misunderstand how this will be used. It is a header-only interoperability layer that is there (and will be installed) independent of whether Kokkos is available on the system or not. Ginkgo (the library) does not need to know whether Kokkos is there, its compilation does not change if Kokkos is there or not, and we only need to detect it for the test and example, because those get compiled like an application would use the Kokkos interoperability layer.

[pratikvn]

@upsj, This Kokkos layer header would still be a part of ginkgo.hpp, right ? Wouldn't the applications then necessarily be compiling with Kokkos (if available), without a way to awitch that off ? Maybe I am still misunderstanding something.

[upsj]

Yes, config.hpp is definitely something that I would prefer to be unchanged, but I was also thinking about Ginkgo::ext::kokkos - is it anything but effectively an alias for Kokkos::kokkos (I would assume that target already sets C++17 requirements?)

[MarcelKoch]

Ok I think I can remove the changes to config.hpp.

Effectively yes, it mainly links to Kokkos::kokkos. The headers are already available, since they are under include/ginkgo. The one extra thing is that Ginkgo::ext links to Ginkgo::ext::kokkos. My intention was that Ginkgo::ext might also link to other extensions, such as json/yaml/whatever parsers. Then it would be easy to activate everything. But thinking about it now, it's a quite unlikely that someone needs to link against the kokkos and parser extension simultaneously.

I think I will remove Ginkgo::ext, it might not be as useful as I thought.

But coming back to the alias issue, for me it is quite hard to think about a way to notify the user of our dependency without the alias target.

[yhmtsai]

agree with the config.hpp was the annoying part. I need to recompile all when I realize I do not install kokkos with complex align

[upsj]

Let me ask this way - how does a user use Kokkos without configuring it in their own build system? They will need to create some views, run some parallel for loops, so they themselves also need to be aware of the Kokkos build system. In the unlikely case that they try to use the Ginkgo Kokkos extension without having provided the correct include paths, we get either a rather clear error message (´#include <Kokkos/...> not found) or can potentially provide our own, more extensive error message using __has_include. That case is unlikely enough to me that making it a build failure to try and use Kokkos would be acceptable to me. But I see your point, getting an error message like unknown target Kokkos::kokkos` at configure/generation would also work, though I don't necessarily consider it more or less useful than the build failure.

[MarcelKoch]

@upsj I agree that is a very clear error message. But it only appears on compile time. By then the user may have spend who knows how long to build and fixing the linking issue may result in them recompiling everything. A configure time error will prevent that very easily.

And even if they already use kokkos, they might not link against it in the required places. Maybe they extracted the parts where they use ginkgo + kokkos into its own library, and then forgot to link again with Kokkos. There are a lot of possibilities for users to mess up. We shouldn't assume that they have the perfect cmake setup and instead complain when we know that they are doing something wrong.

But I also agree that linking Ginkgo::ext::kokkos against kokkos is not the best solution. I only want that there is an error at configure time if a target is linked against Ginkgo::ext::kokkos, but not against kokkos. I will look a bit more into cmake to see if I can get that to work.

[MarcelKoch]

I've now removed the changes to config.hpp. Now the extension should really have zero impact on the core build.

[MarcelKoch]

So far I've not come up with a better solution than linking against kokkos. I think in some sense this is ok, since it is a true dependency. The extension headers really depend on kokkos.

@upsj how should we proceed. I really would like to have a configure time check, so maybe you have a lightweight option? Otherwise if this is a blocker for you then I would remove the cmake setup.

[upsj]

Sorry for holding this up - on second thought, I think the bigger issue to me is that GinkgoConfig.cmake calls find_dependency(Kokkos), not the actual linking step, which kind of precludes the whole linking question.

[MarcelKoch]

@upsj I agree there. I've also thought about removing the dependency (which I would also prefer), but so far, I've not found a solution to provide the minimal Kokkos version we need otherwise.

[upsj]

Just for my understanding: Is there an issue with checking the version beyond compile time vs. configure time?

[MarcelKoch]

No, it's again that issue. If we are not able to have all dependency checks at configure time, we are better off to move everything to compile time, for the benefits you mentioned.

[MarcelKoch]

@upsj I've removed the cmake setup now. There is still the cmake option GINKGO_EXTENSION_KOKKOS_CHECK_TYPE_ALIGNMENT and a corresponding kokkos/config.hpp which contains this option. But besides that, there is no additional cmake setup.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud