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 1 #6200

Merged
merged 14 commits into from
Aug 3, 2023
Merged

Conversation

fnrizzi
Copy link
Contributor

@fnrizzi fnrizzi commented Jun 9, 2023

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

Content

Add team-level support for:

  • Kokkos_AdjacentFind.hpp
  • Kokkos_Count.hpp
  • Kokkos_CountIf.hpp
  • Kokkos_ForEach.hpp
  • Kokkos_ForEachN.hpp
  • Kokkos_LexicographicalCompare.hpp
  • Kokkos_Mismatch.hpp

Public API has now two distinct sections as follows

//
// overload set accepting execution space (as before)
//
template <class ExecutionSpace, /* other templates */>
std::enable_if_t< ::Kokkos::is_execution_space<ExecutionSpace>::value, IteratorType2>
<func_name>(const ExecutionSpace& ex, /*args*/)

// other overloads accepting execution space

------ 

//
// overload set accepting a team handle 
// (all team overloads do not accept a label) 
//
template <class TeamHandleType, /* other templates */>
KOKKOS_FUNCTION
std::enable_if_t<is_team_handle<TeamHandleType>::value>
<func_name>(const TeamHandleType& teamHandle, /*args*/)

// all team level overloads 

Comments

@fnrizzi fnrizzi marked this pull request as draft June 9, 2023 10:50
@fnrizzi fnrizzi changed the title subset of team level impl of std algorithms subset of team level impl of std algorithms (part 1 of N) Jun 9, 2023
@fnrizzi fnrizzi changed the title subset of team level impl of std algorithms (part 1 of N) team-level std algos: subset of non modifying algorithms (part 1 of N) Jun 9, 2023
@fnrizzi fnrizzi changed the title team-level std algos: subset of non modifying algorithms (part 1 of N) team-level std algos: non modifying algorithms - part 1 of N Jun 9, 2023
@fnrizzi fnrizzi changed the title team-level std algos: non modifying algorithms - part 1 of N team-level std algos ( part 1 of N): subset of non modifying algorithms Jun 9, 2023
@fnrizzi fnrizzi changed the title team-level std algos ( part 1 of N): subset of non modifying algorithms team-level std algos (part 1 of N): subset of non modifying algorithms Jun 9, 2023
@fnrizzi fnrizzi changed the title team-level std algos (part 1 of N): subset of non modifying algorithms team-level std algos: part 1 of N Jun 12, 2023
@fnrizzi fnrizzi changed the title team-level std algos: part 1 of N team-level std algos: part 1/9 Jun 12, 2023
@Rombur
Copy link
Member

Rombur commented Jun 12, 2023

Retest this please

@fnrizzi fnrizzi force-pushed the team_level_nonmod_seq_p1 branch 2 times, most recently from e5f7152 to ead775a Compare June 20, 2023 12:17
@fnrizzi fnrizzi marked this pull request as ready for review June 20, 2023 12:27
@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jun 20, 2023

CI failures unrelated (seems to me):

(1) (CUDA 11.6) ccache: error: Failed to write to /tmp/ccache/b/stats.tmp.b69116820fcd.4036.0USVjm
(2) (SYCL) Kokkos_CoreUnitTest_SYCLInterOpStreams (Failed)

@fnrizzi

This comment was marked as outdated.

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jun 22, 2023

CI failure is unrelated:

Sending interrupt signal to process
Sending interrupt signal to process
Terminated
script returned exit code 143

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

fnrizzi commented Jul 4, 2023

CI acting up but was working fine before

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 6, 2023

retest this please

@nliber
Copy link
Contributor

nliber commented Jul 8, 2023

Questions about

template <class ExecutionSpace, /* other templates */>
std::enable_if_t< ::Kokkos::is_execution_space<ExecutionSpace>::value, IteratorType2>
<func_name>(const ExecutionSpace& ex, /*args*/)
  1. Why the absolute ::Kokkos::* namespace? If we are really worried about folks having a nested Kokkos namespace, we really need to address that inside all of Kokkos. And we are already assuming there is no nested std namespace.
  2. With C++17, we now have _v for traits variables.
  3. I prefer just adding an extra enable_if_t parameter instead of conflating the return type into it.
  4. I also prefer typename over class.

In other words, we should write the above as:

template <typename ExecutionSpace, /* other templates */,
          typename = std::enable_if_t<Kokkos::is_execution_space_v<ExecutionSpace>>>
IteratorType2 <func_name>(const ExecutionSpace& ex, /*args*/)

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 17, 2023

@nliber

  1. Why the absolute ::Kokkos::* namespace? If we are really worried about folks having a nested Kokkos namespace, we really need to address that inside all of Kokkos. And we are already assuming there is no nested std namespace.

that was just habit I guess, if you really insist i can drop it

  1. With C++17, we now have _v for traits variables.

Yes, this is just leftover because it was started before we transitioned to 17. I will fix this.

  1. I prefer just adding an extra enable_if_t parameter instead of conflating the return type into it.
  2. I also prefer typename over class.

Ok, i can fix those things , however while trying, I am seeing the compiler complaining in some cases because of the use of typename = std::enable_if_t

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 17, 2023

@nliber here is what i get:

// overload set1
template <
  typename ExecutionSpace, typename IteratorType,
  typename = std::enable_if_t<Kokkos::is_execution_space_v<ExecutionSpace>>
  >
IteratorType adjacent_find(const ExecutionSpace& ex, IteratorType first, IteratorType last){}

// overload set1
template <
  typename TeamHandleType, typename IteratorType,
  typename = std::enable_if_t<Kokkos::is_team_handle_v<TeamHandleType>>
  >
KOKKOS_FUNCTION IteratorType
adjacent_find(const TeamHandleType& teamHandle, IteratorType first, IteratorType last){}

with gcc 9.5.0 i get:

error: redefinition of 'template<class TeamHandleType, class IteratorType, class> IteratorType Kokkos::Experimental::adjacent_find(const TeamHandleType&, IteratorType, IteratorType)'
  126 | adjacent_find(const TeamHandleType& teamHandle, IteratorType first,
      | ^~~~~~~~~~~~~
/Users/fnrizzi/Desktop/work/kokkoscore/team-level-breakapart/pr2/kokkos/algorithms/unit_tests/../src/std_algorithms/Kokkos_AdjacentFind.hpp:35:14: note: 'template<class ExecutionSpace, class IteratorType, class> IteratorType Kokkos::Experimental::adjacent_find(const ExecutionSpace&, IteratorType, IteratorType)' previously declared here
   35 | IteratorType adjacent_find(const ExecutionSpace& ex, IteratorType first, IteratorType last) {

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Jul 17, 2023

whereas if I do this:

template <
  typename ExecutionSpace, typename IteratorType,
  std::enable_if_t<Kokkos::is_execution_space_v<ExecutionSpace>, int > = 0
  >
IteratorType adjacent_find(const ExecutionSpace& ex, IteratorType first, IteratorType last) {}

template <
  typename TeamHandleType, typename IteratorType,
  std::enable_if_t<Kokkos::is_team_handle_v<TeamHandleType>, int > = 0
  >
KOKKOS_FUNCTION IteratorType
adjacent_find(const TeamHandleType& teamHandle, IteratorType first, IteratorType last){}

it works

@crtrott
Copy link
Member

crtrott commented Aug 1, 2023

But this guy just ran recently: #6316

with no such errors.

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 1, 2023

@crtrott @Rombur I just tried to build this on kokkos-dev-2 using nvhpc exactly how the CI is doing it, and I don't get any error and everything works.

This is what i have loaded:

module load gcc/11.1 sems-git/2.29.0 sems-cmake/3.23.1 nvhpc/22.9

which is the same as CI https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/Kokkos/detail/Kokkos/13788/pipeline/48/

and my config line:

cmake -DCMAKE_BUILD_TYPE=<...> \
          -DCMAKE_CXX_COMPILER=nvc++ -DCMAKE_CXX_STANDARD=17 \
          -DKokkos_ARCH_NATIVE=ON -DKokkos_ENABLE_COMPILER_WARNINGS=ON \
          -DKokkos_ENABLE_DEPRECATED_CODE_4=OFF \
          -DKokkos_ENABLE_TESTS=ON -DKokkos_ENABLE_CUDA=ON -DKokkos_ENABLE_CUDA_LAMBDA=ON \
          -DCMAKE_CXX_FLAGS=--diag_suppress=implicit_return_from_non_void_function,no_device_stack \
          -DKokkos_ENABLE_OPENMP=ON \
          -DKokkos_ENABLE_IMPL_MDSPAN=ON \
          -B ${KOKKOS_BUILD_DIR} \
          -S ${KOKKOS_SRC}

output for release, debug, and RelWithDebInfo is attached

team_level_part1_nvhpc_release.txt
team_level_part1_nvhpc_debug.txt
team_level_part1_nvhpc_reldebinfo.txt

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 1, 2023

retest this please

@Rombur
Copy link
Member

Rombur commented Aug 1, 2023

The latest run hits the same error with nvhpc

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 1, 2023

The latest run hits the same error with nvhpc

i know, but i cannot reproduce it: #6200 (comment)

and the PR for part2 passes it for example

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 1, 2023

I think i am going to take for_each and for_each_n and move them into another PR. I think the issue is from the tests not the impl. I dont' see why these would give these weird errors.

@Rombur
Copy link
Member

Rombur commented Aug 1, 2023

Is it the same GPU? The CI is using a V100.

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 1, 2023

Is it the same GPU? The CI is using a V100.

why would the GPU affect the compilation if everything else is the same?

@Rombur
Copy link
Member

Rombur commented Aug 1, 2023

why would the GPU affect the compilation if everything else is the same?

I have no idea how nvhpc works. I don't know if it makes a difference or not.

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 1, 2023

why would the GPU affect the compilation if everything else is the same?

I have no idea how nvhpc works. I don't know if it makes a difference or not.

ah sorry i thought you implied it could make a difference! Don't know either about this

@crtrott
Copy link
Member

crtrott commented Aug 1, 2023

So potentially its around the "Native" can we could try compiling explicitly for AVX

@crtrott
Copy link
Member

crtrott commented Aug 1, 2023

Again failed with Bad OPC :-(

@fnrizzi
Copy link
Contributor Author

fnrizzi commented Aug 2, 2023

3 CI failures are unrelated due to docker inspect failing (nvhpc passes), yuppi yay

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

6 participants