Reviving Kokkos Kernels backend #306

lucbv · 2025-05-23T00:01:29Z

Refreshing some of the implementation of the Kokkos Kernels backend so that we can re-compile and run the tests.
Mostly changing namespaces and a few signatures that changed, especially that of std::extents.
The mdspan_to_view test compiles and fails at runtime because the checks are not correct.
utest_dot_kokkos_exe test compiles and passes.

Note: something is not working with the complex test and I'm choosing to ignore it for the time being but if you have ideas for it let me know...

Let me know if you want the general approach I took to be changed or if modifying the other tests along the same lines would be reasonable?

Some things have changed including various namespaces headers and signatures. With dot seems to compile correctly unfortunately something related to conjugate is failing at compile time. Let's fix that and try to run the dot_kk test. Signed-off-by: Luc Berger-Vergiat <[email protected]>

Not quite sure why overloads are defined for non complex types? Signed-off-by: Luc Berger-Vergiat <[email protected]>

It looks like various changes were made over time and did not necessarily propagate to the Kokkos Kernels implementation which now creates some issues in the namespace hierarchy... Signed-off-by: Luc Berger-Vergiat <[email protected]>

Signed-off-by: Luc Berger-Vergiat <[email protected]>

mhoemmen · 2025-05-28T15:29:24Z

tpl-implementations/include/experimental/__p1673_bits/kokkos-kernels/static_extent_match.hpp

+  return extent1 == Kokkos::dynamic_extent ||
+         extent2 == Kokkos::dynamic_extent ||


For an include file that might be exposed to users, should we consider using the namespace macro instead of hard-coding it as "Kokkos"?

Suggested change

return extent1 == Kokkos::dynamic_extent ||

extent2 == Kokkos::dynamic_extent ||

using MDSPAN_IMPL_STANDARD_NAMESPACE::dynamic_extent;

return extent1 == dynamic_extent ||

extent2 == dynamic_extent ||

Kokkos promises the mdspan types in the Kokkos namespace. In the future these may be aliases to std:: classes but they are not right now (we don't have a full C++26 compatible std implementation, even the C++23 ones which exist can't support GPUs). Since KokkosKernels depends on core I think it's fine to use the Kokkos namespace here.

...mplementations/include/experimental/__p1673_bits/kokkos-kernels/mdspan_to_view_mapper_kk.hpp

mhoemmen · 2025-05-28T15:34:35Z

I wasn't able to comment on the relevant line of code, but one generally can't include <execution> directly (alas). https://github.com/kokkos/stdBLAS/blob/main/include/experimental/__p1673_bits/macros.hpp explains.

#if defined(LINALG_HAS_EXECUTION)
#  include <execution>
#endif

mhoemmen · 2025-05-28T15:35:22Z

tests/kokkos-based/helpers.hpp

 {
  const auto size = v.extent(0);
  if (size == 0) {
    throw std::runtime_error("abs_max() requires non-empty input");
  }
-  const auto i = std::experimental::linalg::vector_idx_abs_max(v);
+  const auto i = MDSPAN_IMPL_STANDARD_NAMESPACE::MDSPAN_IMPL_PROPOSED_NAMESPACE::linalg::vector_idx_abs_max(v);


Please see comments elsewhere on use of namespace macros vs. hard-coding "Kokkos"; thanks!

tests/kokkos-based/mdspan_to_view.cpp

tests/kokkos-based/helpers.hpp

mhoemmen

Thanks for taking this up! : - )

I'm totally fine with merging this after some fixes that I discussed in the comments. In particular, if you could please fix the non-kokkos-kernels test rather than commenting out those lines of code, that would be most excellent; thanks!

For the longer term, we should think about where projects like this should live. The software dependency diagram has an uncomfortable cycle: KokkosCore depends on the C++ Standard Library, kokkos-kernels depends on KokkosCore, std::linalg depends on kokkos-kernels?!?

tests/native/conjugated.cpp

tpl-implementations/include/experimental/__p1673_bits/kokkos-kernels/blas1_dot_kk.hpp

crtrott · 2025-05-28T19:23:13Z

Thanks for taking this up! : - )

I'm totally fine with merging this after some fixes that I discussed in the comments. In particular, if you could please fix the non-kokkos-kernels test rather than commenting out those lines of code, that would be most excellent; thanks!

For the longer term, we should think about where projects like this should live. The software dependency diagram has an uncomfortable cycle: KokkosCore depends on the C++ Standard Library, kokkos-kernels depends on KokkosCore, std::linalg depends on kokkos-kernels?!?

First this isn't actually a standard library implementation here. If (or maybe optimistically "when") we contribute this stuff I am not sure that we could move the KokkosKernels impl with it. But if we did the answer to the dependency thing is simple. std::linalg -> KokkosKernels -> KokkosCore -> std stuff not including linalg (i.e. mdspan and what not). These internal dependencies in the library are not uncommon. The same would be true for calling cuBLAS or oneMKL inside the standard implementation. Both of those also have a dependency on the standard library. Same is already true for stuff like using TBB inside the parallel algorithms implementation.

Next commit will tackle the issues associated with namespaces. Signed-off-by: Luc Berger-Vergiat <[email protected]>

lucbv · 2025-05-28T22:15:29Z

Considering that I include <mdspan/mdspan.hpp> in linalg_kokkoskernels we are guaranteed that MDSPAN_IMPL_STANDARD_NAMESPACE::MDSPAN_IMPL_PROPOSED_NAMESPACE:: will resolve to Kokkos::Experimental. On the other hand I do not mind using the macros to have a smooth transition in case we ever move toward including <experimental/mdspan>.

Signed-off-by: Luc Berger-Vergiat <[email protected]>

mhoemmen · 2025-05-29T04:43:02Z

@lucbv wrote:

Considering that I include <mdspan/mdspan.hpp> in linalg_kokkoskernels we are guaranteed that MDSPAN_IMPL_STANDARD_NAMESPACE::MDSPAN_IMPL_PROPOSED_NAMESPACE:: will resolve to Kokkos::Experimental. On the other hand I do not mind using the macros to have a smooth transition in case we ever move toward including <experimental/mdspan>.

Main thing is to pick one or the other : - )

mhoemmen · 2025-05-29T04:43:53Z

tests/native/conjugated.cpp

@@ -290,10 +290,6 @@ namespace {
  {
    using MDSPAN_IMPL_STANDARD_NAMESPACE :: MDSPAN_IMPL_PROPOSED_NAMESPACE :: linalg::impl::has_conj;

-    static_assert(! has_conj<int>::value);


Just curious -- what was wrong with these static_asserts? Why was this test affected by the kokkos-kernels back-end?

lucbv added 7 commits May 21, 2025 13:12

By passing the test for has_conj as it fails.

f4c23bf

Not quite sure why overloads are defined for non complex types? Signed-off-by: Luc Berger-Vergiat <[email protected]>

Fix more small issues for namespacing and headers location

b8dd6ee

Signed-off-by: Luc Berger-Vergiat <[email protected]>

Update helpers.hpp for namespace and new extents signature

afd749f

Signed-off-by: Luc Berger-Vergiat <[email protected]>

Fixing issues with the mdspan_to_view and dot_kokkos tests

0b52209

Signed-off-by: Luc Berger-Vergiat <[email protected]>

Small clean-ups

e9d90eb

Signed-off-by: Luc Berger-Vergiat <[email protected]>

lucbv requested review from mhoemmen and crtrott May 23, 2025 00:01

mhoemmen reviewed May 28, 2025

View reviewed changes

...mplementations/include/experimental/__p1673_bits/kokkos-kernels/mdspan_to_view_mapper_kk.hpp Show resolved Hide resolved

mhoemmen reviewed May 28, 2025

View reviewed changes

tests/kokkos-based/mdspan_to_view.cpp Outdated Show resolved Hide resolved

mhoemmen reviewed May 28, 2025

View reviewed changes

tests/kokkos-based/mdspan_to_view.cpp Outdated Show resolved Hide resolved

mhoemmen reviewed May 28, 2025

View reviewed changes

tests/kokkos-based/helpers.hpp Outdated Show resolved Hide resolved

mhoemmen requested changes May 28, 2025

View reviewed changes

tests/native/conjugated.cpp Outdated Show resolved Hide resolved

tpl-implementations/include/experimental/__p1673_bits/kokkos-kernels/blas1_dot_kk.hpp Outdated Show resolved Hide resolved

Cleaning code based on reviews

8f555d8

Next commit will tackle the issues associated with namespaces. Signed-off-by: Luc Berger-Vergiat <[email protected]>

Cleaning up namespace utilization based on review comments

5526b0f

Signed-off-by: Luc Berger-Vergiat <[email protected]>

lucbv requested a review from mhoemmen May 28, 2025 23:16

mhoemmen reviewed May 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reviving Kokkos Kernels backend #306

Reviving Kokkos Kernels backend #306

Uh oh!

lucbv commented May 23, 2025

Uh oh!

mhoemmen May 28, 2025

Uh oh!

crtrott May 28, 2025

Uh oh!

Uh oh!

mhoemmen commented May 28, 2025

Uh oh!

mhoemmen May 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhoemmen left a comment

Uh oh!

Uh oh!

Uh oh!

crtrott commented May 28, 2025

Uh oh!

lucbv commented May 28, 2025

Uh oh!

mhoemmen commented May 29, 2025

Uh oh!

mhoemmen May 29, 2025

Uh oh!

Uh oh!

		return extent1 == Kokkos::dynamic_extent \|\|
		extent2 == Kokkos::dynamic_extent \|\|

-  return extent1 == Kokkos::dynamic_extent ||
-         extent2 == Kokkos::dynamic_extent ||
+  using MDSPAN_IMPL_STANDARD_NAMESPACE::dynamic_extent;
+  return extent1 == dynamic_extent ||
+         extent2 == dynamic_extent ||

Reviving Kokkos Kernels backend #306

Are you sure you want to change the base?

Reviving Kokkos Kernels backend #306

Uh oh!

Conversation

lucbv commented May 23, 2025

Uh oh!

mhoemmen May 28, 2025

Choose a reason for hiding this comment

Uh oh!

crtrott May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhoemmen commented May 28, 2025

Uh oh!

mhoemmen May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhoemmen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

crtrott commented May 28, 2025

Uh oh!

lucbv commented May 28, 2025

Uh oh!

mhoemmen commented May 29, 2025

Uh oh!

mhoemmen May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!