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

Implement conversion helpers between Kokkos::Array and std::array #170

Open
tpadioleau opened this issue Oct 11, 2024 · 9 comments
Open

Comments

@tpadioleau
Copy link
Collaborator

We have many of these conversions:

Kokkos::Array<int, 3> map = {_map[0], _map[1], _map[2]};

Kokkos::Array<int, rank> map = {_map[0], _map[1], _map[2], _map[3]};

Kokkos::Array<int, rank> map = {_map[0], _map[1], _map[2], _map[3], _map[4]};

...

It would be helpful to implement once and for all a helper to go back and forth between Kokkos::Array and std::array. It could even be an upstream proposal for Kokkos core.

@dalg24
Copy link
Member

dalg24 commented Oct 11, 2024

https://godbolt.org/z/3vfv34MK3

@tpadioleau
Copy link
Collaborator Author

@dalg24 do you think this can go to Kokkos 4.5 ?

@dalg24
Copy link
Member

dalg24 commented Oct 11, 2024

I am torn on this. I don't mind too much the to_array free functions but I am afraid others will soon come and ask for regular conversion operations and I am strongly against these.
We already have some of that mess in Kokkos::complex in the name of "compatibility".

@tpadioleau
Copy link
Collaborator Author

I would be happy to have these free functions, even for other projects. That could also be seen as helper tools in porting existing C++ codes to GPU.

About conversion operations we could argue that we only want to support at most the API of std::array.

@dalg24
Copy link
Member

dalg24 commented Oct 11, 2024

Kokkos::to_array(std::array) -> Kokkos::Array looks like the obvious choice but what did you have in mind for the conversion Kokkos:: to std::array?
Kokkos::to_std_array(Kokkos::Array) -> std::array?
Apparently Kokkos::pair has a to_std_pair() member function
https://github.com/kokkos/kokkos/blob/abcb3dce75effd5cd7a2d73e9e51a9202573acab/core/src/Kokkos_Pair.hpp#L144-L146

@tpadioleau
Copy link
Collaborator Author

tpadioleau commented Oct 11, 2024

Yes seems fine

namespace Kokkos
{
    // 1.
    template <class T, std::size_t N>
    using array = Kokkos::Array<T, N>;

    // 2.
    template <class T, std::size_t N>
    array<T, N> to_array(std::array<T, N> const&/&& x)

    // 3.
    template <class T, std::size_t N>
    array<T, N> to_array(T (const&/&& x) [N])

    // 4.
    // maybe a member function to be coherent with the existing `to_std_pair`
    template <class T, std::size_t N>
    std::array<T, N> to_std_array(array<T, N> const&/&& x)

    // 5.
    template <class T1, class T2>
    std::pair<T1, T2> to_std_pair(pair<T1, T2> const&/&& x)
}

What do you think of that ?

@dalg24
Copy link
Member

dalg24 commented Oct 11, 2024

Alias: I am not loving the uppercase A either but I wonder if that would not just add more to confusion. Remember that there is no CTAD on aliases in C++17.

to_array free function overloads: conversion from C arrays is already there.
https://github.com/kokkos/kokkos/blob/abcb3dce75effd5cd7a2d73e9e51a9202573acab/core/src/Kokkos_Array.hpp#L388-L396
I would want to think more about the const-qualifier and I would say no to the member function.

to_std_pair: I would say no unless someone actually ask for it or if you volunteer to document both pair and array ;)

@tpadioleau
Copy link
Collaborator Author

tpadioleau commented Oct 13, 2024

Alias: I am not loving the uppercase A either but I wonder if that would not just add more to confusion. Remember that there is no CTAD on aliases in C++17.

What do you think of deprecating Kokko::Array in favor of Kokkos::array ? This alias could be used in 4.5 and make Kokkos::Array the alias in 5.0 ?

to_array free function overloads: conversion from C arrays is already there. https://github.com/kokkos/kokkos/blob/abcb3dce75effd5cd7a2d73e9e51a9202573acab/core/src/Kokkos_Array.hpp#L388-L396 I would want to think more about the const-qualifier and I would say no to the member function.

Are you wondering about the const qualifier for the C-array or std::array ? The current implementation seems to handle fine the const qualifier for C-array so proposition 3. is no more necessary.

to_std_pair: I would say no unless someone actually ask for it or if you volunteer to document both pair and array ;)

The idea is to be as close as possible to std::array and std::pair so that the documentation would be "go see cppref, Kokkos only make them GPU compatible". Of course we would need to document functions that allow conversions between standard and Kokkos implementations.

@dalg24
Copy link
Member

dalg24 commented Oct 14, 2024

What do you think of deprecating Kokko::Array in favor of Kokkos::array ? This alias could be used in 4.5 and make Kokkos::Array the alias in 5.0 ?

Not sure I understand. You want template <...> using array = Array<...>; in 4.5 and in 5.0 we swap them template <...> using Array [[deprecated]] = array<...>;?

Are you wondering about the const qualifier for the C-array or std::array ? The current implementation seems to handle fine the const qualifier for C-array so proposition 3. is no more necessary.

You had a const-qualifier in your to_array(std::array const&) -> Kokkos::Array, I suspect we really want it non-const.

The idea is to be as close as possible to std::array and std::pair so that the documentation would be "go see cppref, Kokkos only make them GPU compatible". Of course we would need to document functions that allow conversions between standard and Kokkos implementations.

Right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants