Skip to content

[Mirror] [5.1.1] 0 simd: Fix compile failures when ranges are not available - #9

Open
csiefer2 wants to merge 11 commits into
developfrom
pr-mirror-9111
Open

[Mirror] [5.1.1] 0 simd: Fix compile failures when ranges are not available#9
csiefer2 wants to merge 11 commits into
developfrom
pr-mirror-9111

Conversation

@csiefer2

Copy link
Copy Markdown
Owner

Automated mirror of upstream PR kokkos#9111 Cherry-pick of kokkos#9065 onto release-candidate-5.1.1.

Original PR: kokkos#9065

dalg24 and others added 11 commits March 4, 2026 18:38
Signed-off-by: Damien L-G <dalg24@gmail.com>
Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
```
/path/to/kokkos/core/src/Kokkos_Macros.hpp:36:6: warning: 'KOKKOS_VERSION' is not defined, evaluates to 0 [-Wundef]
   36 | #if !KOKKOS_VERSION_EQUAL(KOKKOS_VERSION_MAJOR, KOKKOS_VERSION_MINOR, \
      |      ^
/path/to/kokkos/core/src/Kokkos_Macros.hpp:34:4: note: expanded from macro 'KOKKOS_VERSION_EQUAL'
   34 |   (KOKKOS_VERSION == ((MAJOR)*10000 + (MINOR)*100 + (PATCH)))
      |    ^
/path/to/kokkos/core/src/Kokkos_Macros.hpp:36:27: warning: 'KOKKOS_VERSION_MAJOR' is not defined, evaluates to 0 [-Wundef]
   36 | #if !KOKKOS_VERSION_EQUAL(KOKKOS_VERSION_MAJOR, KOKKOS_VERSION_MINOR, \
      |                           ^
/path/to/kokkos/core/src/Kokkos_Macros.hpp:36:49: warning: 'KOKKOS_VERSION_MINOR' is not defined, evaluates to 0 [-Wundef]
   36 | #if !KOKKOS_VERSION_EQUAL(KOKKOS_VERSION_MAJOR, KOKKOS_VERSION_MINOR, \
      |                                                 ^
/path/to/kokkos/core/src/Kokkos_Macros.hpp:37:27: warning: 'KOKKOS_VERSION_PATCH' is not defined, evaluates to 0 [-Wundef]
   37 |                           KOKKOS_VERSION_PATCH)
      |                           ^
```

Co-Authored-By: Daniel Arndt <arndtd@ornl.gov>
Signed-off-by: Damien L-G <dalg24@gmail.com>
[5.1.0] Fix -Wundef warnings about KOKKOS_VERSION* not being defined
[5.1.0] Identify homebrew llvm as KOKKOS_COMPILER_CLANG
Signed-off-by: Seyong Lee <lees2@ornl.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Co-authored-by: Jakob Bludau <104908666+JBludau@users.noreply.github.com>
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
Co-authored-by: Nathan Ellingwood <ndellin@sandia.gov>
Co-authored-by: Trévis Morvany <63788850+tretre91@users.noreply.github.com>
Co-authored-by: Christian Trott <crtrott@sandia.gov>
…8967)

* Fix subview construction between managed and unmanaged Views
* Add test

Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
Signed-off-by: Damien L-G <dalg24@gmail.com>
…ubview_constructor

[5.1.0] Fix subview construction between managed and unmanaged Views
Signed-off-by: Damien L-G <dalg24@gmail.com>
Adds a minimal set of ranges support necessary to make range based ctors of SIMD types work.
The helper classes and concepts are not publicly exposed, and only used inside of the simd
sublibrary.

Signed-off-by: Trévis Morvany <trevis.morvany@cea.fr>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Co-authored-by: Christian Trott <crtrott@sandia.gov>
(cherry picked from commit 8bbe702)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request bumps the Kokkos version to 5.1.1 and updates the CHANGELOG for the 5.1.0 release. Key technical changes include the introduction of a fallback implementation for C++20 ranges within the SIMD library to support compilers with incomplete ranges support (such as Clang 14/15), the addition of a static_cast for pointer offsets in BasicView, and new unit tests for unmanaged subview construction. Feedback focuses on improving the implementation of the ranges fallback, specifically suggesting a more standard approach for concept definitions to handle Argument-Dependent Lookup and refining iterator arithmetic checks in concept requirements.

Comment on lines +42 to +49
concept range = []() {
using std::begin;
using std::end;
return requires(R& r) {
begin(r);
end(r);
};
}();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The use of an immediately-invoked lambda to define a concept is highly unconventional and may lead to portability issues or confusion. A more standard approach to handle Argument-Dependent Lookup (ADL) for begin and end within a concept is to use a helper namespace.

namespace detail {
using std::begin;
using std::end;
template <class T>
concept range_impl = requires(T& t) {
  begin(t);
  end(t);
};
}
template <class R>
concept range = detail::range_impl<R>;

Comment on lines +79 to +82
{ it + 2 } -> std::same_as<Impl::iterator_t<R> >;
{ it - 2 } -> std::same_as<Impl::iterator_t<R> >;
{ it += 2 } -> std::same_as<Impl::iterator_t<R>&>;
{ it -= 2 } -> std::same_as<Impl::iterator_t<R>&>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using an offset of 2 in the iterator arithmetic checks is arbitrary. Standard practice for verifying random access iterator properties usually involves offsets of 1 or n.

Suggested change
{ it + 2 } -> std::same_as<Impl::iterator_t<R> >;
{ it - 2 } -> std::same_as<Impl::iterator_t<R> >;
{ it += 2 } -> std::same_as<Impl::iterator_t<R>&>;
{ it -= 2 } -> std::same_as<Impl::iterator_t<R>&>;
{ it + 1 } -> std::same_as<Impl::iterator_t<R>>;
{ it - 1 } -> std::same_as<Impl::iterator_t<R>>;
{ it += 1 } -> std::same_as<Impl::iterator_t<R>&>;
{ it -= 1 } -> std::same_as<Impl::iterator_t<R>&>;

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.

5 participants