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

Move intelligence to Impl::create_mirror* #7

Open
wants to merge 20 commits into
base: refactor/create-mirror/constexpr
Choose a base branch
from

Conversation

pzehner
Copy link
Member

@pzehner pzehner commented Mar 21, 2024

This PR aims to simplify the code by moving the duplicated intelligence of create_mirror and create_mirror_view to Impl::create_mirror and Impl::create_mirror_view. The former functions now behave as a pass-trough that only call their Impl:: counterpart.

This PR is part of a of a larger effort which aims to fix kokkos#6842.

A functor CopyUnInit is implemented in the test file core/unit_test/TestViewAPI.hpp which contains a View as an attribute whose type is hard-coded using a MirrorViewType implementation object here (it is the only test in the file containing an implementation object). The type expected by the functor is a view whose Space parameter corresponds to the template Space indicated in the same functor here.

In the develop branch, the overload of the create_mirror function called in the test imposes the following return type: Impl::MirrorViewType<Space, T, P...>::view_type which matches the type expected by the constructor of the functor in the test.

In this branch, the return types of the functions are auto. Therefore, the return value is decided at a single place. The overload of create_mirror_view called calls the function Impl::create_mirror_view, which in turn calls the function create_mirror, which returns the following type: Impl::MirrorType<memory_space, T, P...>::view_type that does not match the type expected by the constructor of the test because of the Space::memory_space passed as a Space parameter to the output View.
It should be noted that in the develop branch, several return values are set, but they are different.

To fix the mismatch of types encountered with this PR, we decided to simplify the test code by removing the hard-coded type and simply deducing the object's type by passing it to the constructor. It should be noted that this functor is not used anywhere else in the code.

Tests:

  • Clang 10.0.0 with CUDA 11.0;
  • Clang 8.0.0 with OpenMP;
  • GCC 8.2.0 8.4.0 with OpenMP;
  • GCC 8.2.0 8.4.0 with CUDA 11.0 11.4;
  • Intel Classic 19.0.5 2023.2.1 with OpenMP;
  • Intel LLVM 2022.0.0 2024.0.2 with SYCL;
  • Intel LLVM 2021.1.1 2023.2.1 with OpenMP;
  • MSVC 19.29;
  • NVHPC/PGI 22.3; and
  • ROCM 5.2.0 with HIP.

@pzehner pzehner marked this pull request as ready for review March 22, 2024 15:37
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure to run clang-format8.

core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved
Comment on lines 3602 to 3567
typename Kokkos::View<T, P...>::HostMirror create_mirror_view(
auto create_mirror_view(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure that auto works properly on all platforms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should run all the different backends with the minimum supported compilers. So far, we could build for Serial, CUDA and SYCL (latest versions of compilers, mostly).

core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

3 participants