Replace SFINAE by `if constexpr` for `create_mirror*` #6

pzehner · 2024-03-21T17:39:50Z

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:

Typo in loop indices, from wrong copy-paste.

Disabling call to value checker as it is not supported by the backend.

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

…onstexpr

cedricchevalier19

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 · 2024-03-27T16:46:59Z

Ok, will try the different compilers.

* Cuda: Fix configuring with CMake 3.29.0 * CMake 3.28.4 is also affected

Fix Makefile.kokkos for Threads

* 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

tpadioleau

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

tpadioleau · 2024-04-02T07:32:39Z

core/src/Kokkos_CopyViews.hpp

-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,


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.

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

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.

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.

…or_each

pzehner · 2024-04-02T10:28:13Z

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.

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 Copyright file

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.

Remove cuSPARSE TPL

* 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

Rombur and others added 10 commits February 2, 2024 14:42

Reenable TestHIP_Memory_Requirements

b4bc406

Don't use Fedora development version in GitHub CI

c1a8006

Fix deep copy when filling Rank-7 views

841b3a9

Typo in loop indices, from wrong copy-paste.

TestViewCopy_c.hpp: add new unit test for deep copy (ViewFill)

a2f2ba4

TestViewCopy_c.hpp: better handling for OpenMPTarget

ae4d001

Disabling call to value checker as it is not supported by the backend.

Use builtin for atomic_fetch in the HIP backend

46354d2

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

Combine the two Impl::create_mirror functions into one with constexpr

70603df

Format create_mirror

6524e2b

Combine the four Impl::create_mirror_view in one using if constexpr

1635ff8

Combine the two create_mirror_view_and_copy functions into one with c…

d3bb03a

…onstexpr

pzehner assigned pzehner and thierryantoun Mar 21, 2024

pzehner changed the title ~~Replace SFINAE by if constexpr~~ Replace SFINAE by if constexpr for create_mirror* Mar 21, 2024

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

Fix Makefile.kokkos for Threads

872dc42

cedricchevalier19 reviewed Mar 25, 2024

View reviewed changes

Fix a bug in Makefile when using AMD GPU architectures (kokkos#6892)

2035e31

* Fix bug in Makefile when using AMD GPU architectures * Fix indentation * Fix documentation of the architecture to match the code

masterleinad and others added 8 commits March 27, 2024 23:08

Cuda: Fix configuring with CMake 3.28.4 (kokkos#6898)

8d734b0

* Cuda: Fix configuring with CMake 3.29.0 * CMake 3.28.4 is also affected

Merge pull request kokkos#6896 from masterleinad/fix_makefile_threads

a53d30a

Fix Makefile.kokkos for Threads

Update Intel GPU architectures in Makefile (kokkos#6895)

68c6684

* Update Intel GPU architectures in Makefile * Add some comments

Drop Experimental::LayoutTiled class template

e2cfdec

Get rid of now unnecessary use of is_layouttiled trait

51b98e1

Deprecate is_layouttiled trait

1efeb5d

Merge pull request kokkos#6876 from masterleinad/disable_fedora_rawhide

5eac0bc

Don't use Fedora development version in GitHub CI

Move Kokkos::Array tests to a more suitable place (kokkos#6905)

6355510

* Move Kokkos::Array tests to a more suitable place * Workaround bogous(?) compile error with Array::operator[] not being constexpr

tpadioleau requested changes Apr 2, 2024

View reviewed changes

Do not return a copy of the input functor for Kokkos::Experimental::f…

391e040

…or_each

Drop specialization of ViewMapping for Kokkos::Array

b667853

dalg24 and others added 30 commits May 1, 2024 10:37

Merge pull request kokkos#6949 from Rombur/nightly_deprecated

27b3ced

Enable deprecated code and deprecated warnings in nightly CI

Merge pull request kokkos#6882 from Rombur/hip_atomic_fetch

15d13f2

DESUL: Use builtin atomics in the HIP backend

Merge pull request kokkos#6972 from dalg24/fix_kokkos_compile_languag…

ed4d254

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

Merge pull request kokkos#6962 from dalg24/kokkos_array_const_qualifi…

dbd7f58

…ed_element_type Fix support for `Kokkos::Array` of const-qualified element type

Merge branch 'develop' into refactor/create-mirror/constexpr

1c8b624

Fix fedora CI builds with flang-new

ccd0126

Fix Copyright file

45a1404

Merge pull request kokkos#6984 from crtrott/Copyright

85610f4

Fix Copyright file

Merge pull request kokkos#6982 from masterleinad/fix_fedora

a75dc70

Fix fedora CI builds with flang-new

Also use is_nothrow_swappable workaround for Intel Classic Compilers (k…

c6d8647

…okkos#6983) * Also use is_nothrow_swappable workaround for Intel Classic Compilers * Use template parameter U directly in kokkos_swap overload

Merge branch 'develop' into refactor/create-mirror/constexpr

b29b14f

Add thread-safety tests (kokkos#6938)

69567f3

* 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

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

9c79202

Co-Authored-By: Andrey Prokopenko <prokopenkoav@ornl.gov>

Fix TPL_LIBRARY_SUFFIXES for 32-bit build

7b8e3a6

Avoid duplicated definition of KOKKOS_IMPL_32BIT

2826017

Disable failing parallel_scan_with_reducers test

ccadc7d

Merge pull request kokkos#6989 from dalg24/deprecated_attribute_compa…

06e4c5b

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

Merge pull request kokkos#6990 from masterleinad/fix_32bit_tpl_librar…

e4cc686

…y_path Fix TPL_LIBRARY_SUFFIXES for 32-bit build

SYCL: Fix deprecation in custom parallel_for RangePolicy implementation

f5b3422

[ci skip] update changelog for 4.3.1 (kokkos#6995)

37986fd

* [ci skip] update changelog for 4.3.1 * changelog: fixup

OpenMPTarget: Use mutex lock for parallel scan.

7cad3e7

Merge pull request kokkos#6998 from rgayatri23/ompt_scan_lock

a69e81a

OpenMPTarget: Use mutex lock for parallel scan.

Merge pull request kokkos#6997 from masterleinad/sycl_fix_custom_para…

5a5306c

…llel_for_range_deprecations SYCL: Fix deprecation in custom parallel_for RangePolicy implementation

Remove cuSPARSE TPL

00170ae

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.

Merge pull request kokkos#7002 from dalg24/rm_tpl_cusparse

506da18

Remove cuSPARSE TPL

Merge branch 'develop' into refactor/create-mirror/constexpr

be07ebc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace SFINAE by `if constexpr` for `create_mirror*` #6

Replace SFINAE by `if constexpr` for `create_mirror*` #6

pzehner commented Mar 21, 2024 •

edited

Loading

cedricchevalier19 left a comment

pzehner commented Mar 27, 2024

tpadioleau left a comment

tpadioleau Apr 2, 2024

pzehner Apr 2, 2024

tpadioleau Apr 2, 2024

pzehner Apr 19, 2024 •

edited

Loading

pzehner commented Apr 2, 2024 •

edited

Loading

Replace SFINAE by if constexpr for create_mirror* #6

Are you sure you want to change the base?

Replace SFINAE by if constexpr for create_mirror* #6

Conversation

pzehner commented Mar 21, 2024 • edited Loading

cedricchevalier19 left a comment

Choose a reason for hiding this comment

pzehner commented Mar 27, 2024

tpadioleau left a comment

Choose a reason for hiding this comment

tpadioleau Apr 2, 2024

Choose a reason for hiding this comment

pzehner Apr 2, 2024

Choose a reason for hiding this comment

tpadioleau Apr 2, 2024

Choose a reason for hiding this comment

pzehner Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

pzehner commented Apr 2, 2024 • edited Loading

Replace SFINAE by `if constexpr` for `create_mirror*` #6

Replace SFINAE by `if constexpr` for `create_mirror*` #6

pzehner commented Mar 21, 2024 •

edited

Loading

pzehner Apr 19, 2024 •

edited

Loading

pzehner commented Apr 2, 2024 •

edited

Loading