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

team-level std algos: part 2 #6205

Merged
merged 15 commits into from
Aug 24, 2023
Merged

Conversation

fnrizzi
Copy link
Contributor

@fnrizzi fnrizzi commented Jun 10, 2023

Co-authored-by: Cezary Skrzyński
Co-authored-by: Jakub Strzebonski

Content

Team-level implementation and tests for:

  • Kokkos_Equal.hpp
  • Kokkos_Search.hpp
  • Kokkos_FindEnd.hpp
  • Kokkos_FindFirstOf.hpp

Technical comments

  • the change class -> typename and the enable if defaulted in the template parameters comes from this request

  • the code duplication is intentional and agreed upon after the long discussion in here

  • tests are run for both contiguous views and strided views

Other comments

Possible conflicts

Implementation-wise, this has not conflict with other team-level PRs so it could be merged independently.
However, a trivial conflict in the CMakeLists inside the unit test occurs if another of the team-level PRs is merged before this because each team-level PR adds test using a specific "label". Resolving this is trivial.

Associated wiki PRs

IMPORTANT: wiki PRs are not complete yet but 99% there

The issue tracking all wiki PRs is here

@fnrizzi fnrizzi marked this pull request as draft June 10, 2023 19:46
@fnrizzi fnrizzi changed the title team-level std algos (part 2 of N): subset of non modifying algorithms team-level std algos: part 2 of N Jun 12, 2023
@fnrizzi fnrizzi changed the title team-level std algos: part 2 of N team-level std algos: part 2/9 Jun 12, 2023
Co-authored-by: Cezary Skrzyński
Co-authored-by: Jakub Strzebonski
@fnrizzi fnrizzi marked this pull request as ready for review June 20, 2023 12:38
@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jun 20, 2023

CI failure unrelated

@fnrizzi

This comment was marked as outdated.

@fnrizzi fnrizzi changed the title team-level std algos: part 2/9 team-level std algos: part 2 Jul 4, 2023
@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 4, 2023

ci acting up but was fine before

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 24, 2023

retest this please

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 27, 2023

retest this please

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 2, 2023

all CI failures are unrelated

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 8, 2023

only hvhpc fails with compiler error opc

const auto s_count = KE::distance(s_first, s_last);
namespace KE = ::Kokkos::Experimental;
const auto num_elements = KE::distance(first, last);
[[maybe_unused]] const auto s_count = KE::distance(s_first, s_last);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this always used (in line 115)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the complain comes from line 106, it sees this possibility of existing earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the same should be true for num_elements though , but don't recall seeing that warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to remove them again

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know.

algorithms/src/std_algorithms/impl/Kokkos_FindEnd.hpp Outdated Show resolved Hide resolved
// the target sequence should not be larger than the range [first, last)
namespace KE = ::Kokkos::Experimental;
const auto num_elements = KE::distance(first, last);
[[maybe_unused]] const auto s_count = KE::distance(s_first, s_last);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this unused? Which compiler complains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the complain comes from line 189 and 193, it sees this possibility of existing earlier. I don't remember which compiler is doing this but definitely was getting warnings. I can dig it back up if you really want to know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the same should be true for num_elements though

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 18, 2023

@masterleinad I removed all the maybe_unused and it seems like CI works without complaining (unless the warning is somewhere and not treated as error but we use -Werror everywhere right?)

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Fine with me.

template <class ExecutionSpace, class IteratorType1, class IteratorType2>
template <
typename ExecutionSpace, typename IteratorType1, typename IteratorType2,
std::enable_if_t<::Kokkos::is_execution_space_v<ExecutionSpace>, int> = 0>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these guys are not constrained on are_iterators like the equal algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the different # of params does not cause the same ambiguous error from compiler

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.

None yet

4 participants