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

Replace SFINAE by if constexpr for create_mirror* #6

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

Conversation

pzehner
Copy link
Member

@pzehner pzehner commented Mar 21, 2024

This PR is a refactor without side effects.

The idea is to simplify the code of create_mirror* functions by applying techniques of the C++ 17 standard. A refactor with the similar approach was drafted by @masterleinad in kokkos#5533.

The multiple overloaded functions of Impl::create_mirror, Impl::create_mirror_view, and Impl::create_mirror_view_and_copy that use SFINAE are replaced by one function that uses if constexpr instead (for Kokkos views, offset views, dynamic views and dynamic rank views). Eventually, the return type of these functions is replaced by auto, and decided depending on which branch of the if constexpr is used.

The logic of the public create_mirror* functions (which is sometimes redundant with the logic of the private functions) is not altered in this PR and will be assessed in an upcoming one.

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

Built and tested successfully for:

  • 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 changed the title Replace SFINAE by if constexpr Replace SFINAE by if constexpr for create_mirror* Mar 21, 2024
@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.

Please try to run on most of the backends and with Kokkos testing configuration.
I had some weird behavior with constexpr on some architectures.

* Fix bug in Makefile when using AMD GPU architectures

* Fix indentation

* Fix documentation of the architecture to match the code
@pzehner
Copy link
Member Author

pzehner commented Mar 27, 2024

Ok, will try the different compilers.

masterleinad and others added 8 commits March 27, 2024 23:08
* Cuda: Fix configuring with CMake 3.29.0

* CMake 3.28.4 is also affected
* Update Intel GPU architectures in Makefile

* Add some comments
Don't use Fedora development version in GitHub CI
* Move Kokkos::Array tests to a more suitable place

* Workaround bogous(?) compile error with Array::operator[] not being constexpr
Copy link
Member

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

Looks good to me. I wonder if we could also if constexpr-ise the create_mirror_view between lines 3575 and 3599 in the new version ?

core/src/Kokkos_CopyViews.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_CopyViews.hpp Show resolved Hide resolved
create_mirror_view(const Kokkos::View<T, P...>& src,
const Impl::ViewCtorProp<ViewCtorArgs...>& arg_prop) {
return Kokkos::Impl::create_mirror(src, arg_prop);
auto create_mirror_view(const Kokkos::View<T, P...>& src,
Copy link
Member

Choose a reason for hiding this comment

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

I notice two changes:

  • the use of auto,
  • some functions were marked inline.

I would suggest to mention these changes when you open a PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The inline functions were also templated, so I wonder if the keyword was necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Well the keyword is primarily a hint for the compiler to actually inline a function call before avoiding an ODR violation.

Personally I would remove the keyword in this case.

Copy link
Member Author

@pzehner pzehner Apr 19, 2024

Choose a reason for hiding this comment

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

I changed my mind about the inline keyword, and let its use unaltered. According to this SO answer, function templates are not inlined by default. This change was outside the scope of this PR.

@pzehner
Copy link
Member Author

pzehner commented Apr 2, 2024

I wonder if we could also if constexpr-ise the create_mirror_view between lines 3575 and 3599 in the new version ?

The logic of the public create_mirror_view functions (which is redundant with the logic of the private functions) is not altered in this PR. We change it in #7 and eventually remove it, so there is no need to if constexpr-ise more.

dalg24 and others added 30 commits May 1, 2024 10:37
Enable deprecated code and deprecated warnings in nightly CI
DESUL: Use builtin atomics  in the HIP backend
…e_cuda_hip_w_omp

Fix use of OpenMP with Cuda or HIP as compile language
…ed_element_type

Fix support for `Kokkos::Array` of const-qualified element type
Fix fedora CI builds with flang-new
…okkos#6983)

* Also use is_nothrow_swappable workaround for Intel Classic Compilers

* Use template parameter U directly in kokkos_swap overload
* Add thread-safety tests

* Disable thread-safety tests for Serial and OpenMP for now

* Cleanup include and namespace

* Skip tests for OpenACC in CMakeLists.txt

* Avoid std::move

* Comment on tests

* Use more atomics

* Simplify test
Co-Authored-By: Andrey Prokopenko <prokopenkoav@ornl.gov>
…rison_operators_pair_t1_void

Fix deprecation warnings with GCC for `pair<T1,void>` comparison operators
…y_path

Fix TPL_LIBRARY_SUFFIXES for 32-bit build
…ement types (kokkos#6993)

* Add few missing constexpr for alloc_prop_input

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

* Update tests

* Fix DualView

* Address review comments

* Add missing decorators

* Move NoDefaultConstructor out of function

---------

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
* SYCL: Make Parallel* copyable

* Address review comments

* Refactor Team policies further

* Fix alias for SYCL TeamPolicy ParallelReduce

* Improve const-correctness in Kokkos_SYCL_ParallelReduce_Team

* Fix up Kokkos_SYCL_ParallelReduce_Team.hpp
* [ci skip] update changelog for 4.3.1

* changelog: fixup
OpenMPTarget: Use mutex lock for parallel scan.
…llel_for_range_deprecations

 SYCL: Fix deprecation in custom parallel_for RangePolicy implementation
It looks like an oversight.  It is unused.  Example code that referred
to it was removed in kokkos#2688 because it was just sitting there, i.e. not
built nor tested.

KokkosKernels has its own CMake logic to find it and link against it.
* SYCL: Print submission command queue property

* Also print SYCL_PI_LEVEL_ZERO_USE_IMMEDIATE_COMMANDLISTS if set

* Reword printout for environment variable

Co-authored-by: Damien L-G <dalg24+github@gmail.com>

---------

Co-authored-by: Damien L-G <dalg24+github@gmail.com>
…#6999)

* Introduce `KOKKOS_IMPL_DISABLE_DEPRECATED_WARNINGS_{PUSH,POP} macros

to suppress diagnostics when appropriate

* Suppress all deprecated warnings I can see in tests

* Update EDG diag suppress to fix the Intel Compiler Classic

and provide a fallback empty definition for the macros
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.