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

Kokkos 3.1 #268

Merged
merged 7 commits into from
Apr 18, 2020
Merged

Kokkos 3.1 #268

merged 7 commits into from
Apr 18, 2020

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Apr 16, 2020

The main motivation for updating to 3.1 it to allow HIP backend. We could have done magic to still work with 3.0, but we decided not to bear cost of supporting that.

@masterleinad
Copy link
Collaborator

What is the rationale for having this in addition to #236?

@aprokop
Copy link
Contributor Author

aprokop commented Apr 16, 2020

Introducing HIP backend and fixing corresponding issues is separate from moving ArborX to Kokkos 3.1.

@aprokop aprokop added build Build and installation documentation testing Anything to do with tests and CI labels Apr 16, 2020
@masterleinad
Copy link
Collaborator

Introducing HIP backend and fixing corresponding issues is separate from moving ArborX to Kokkos 3.1.

I am happy if you cherry-pick all the other changes from that branch that are not related to HIP.

@masterleinad
Copy link
Collaborator

Retest this please.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 16, 2020

I don't understand why it passes. If I look at, say, CUDA-10.1-NVCC build, I see

/var/jenkins/workspace/ArborX_PR-268/src/details/ArborX_Predicates.hpp(34): warning: __device__ annotation is ignored on a function("Nearest") that is explicitly defaulted on its first declaration

in the output. But Style check still passes.

@masterleinad
Copy link
Collaborator

I would not be surprised if it doesn't catch warnings coming from the device compiler.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 16, 2020

Can you please fix those?

@masterleinad
Copy link
Collaborator

We still have

/opt/boost/include/boost/fusion/container/vector/vector.hpp(189): warning: __host__ annotation is ignored on a function("vector_data") that is explicitly defaulted on its first declaration

but I would just not care about the boost warnings at this point.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 16, 2020

Thanks. I noticed that too. In CUDA build the thing that produces the error

/opt/kokkos/bin/nvcc_wrapper  -Wpedantic -Wall -Wextra -g  -fopenmp -mavx CMakeFiles/ArborX_DistributedTree.exe.dir/distributed_tree_driver.cpp.o  -o ArborX_DistributedTree.exe -Wl,-rpath,/opt/boost/lib:/opt/openmpi/lib /opt/boost/lib/libboost_program_options.so /opt/kokkos/lib/libkokkoscontainers.a /opt/kokkos/lib/libkokkoscore.a -lcuda /usr/lib/x86_64-linux-gnu/libdl.so -Wl,-rpath -Wl,/opt/openmpi/lib -Wl,--enable-new-dtags -pthread /opt/openmpi/lib/libmpi.so 

For some reason I don't see any -isystem include directories here. But I see them in other places. Weird.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 17, 2020

Rebased on top of master. Temporarily removed defaulted function patch to see the reaction of CI on it.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 17, 2020

Interestingly, even when you have warnings, they are not being caught by -Wextra:

[9/88] ccache /home/xap/local/opt/kokkos/bin/nvcc_wrapper  -DBOOST_TEST_DYN_LINK -I../src -I../src/details -Iinclude -isystem /home/xap/local/opt/kokkos/include -isystem /home/xap/local/opt/spack/opt/spack/linux-ubuntu18.04-broadwell/gcc-7.4.0/openmpi-3.1.4-oetr45yq7vnbcqhiiaalfkpoftuujlzn/include -isystem /home/xap/local/opt/spack/opt/spack/linux-ubuntu18.04-broadwell/gcc-7.4
.0/boost-1.68.0-syfratmpslzh7vpultgho6h54mwh4g34/include -lineinfo -DKOKKOS_ENABLE_PROFILING -Wall -Wextra -Werror -O2 -g -DNDEBUG   -expt-extended-lambda -arch=sm_61 -Xcompiler -fopenmp -fdiagnostics-color -std=c++14 -MD -MT test/CMakeFiles/ArborX_DetailsContainers.exe.dir/tstSequenceContainers.cpp.o -MF test/CMakeFiles/ArborX_DetailsContainers.exe.dir/tstSequenceContainers.c
pp.o.d -o test/CMakeFiles/ArborX_DetailsContainers.exe.dir/tstSequenceContainers.cpp.o -c ../test/tstSequenceContainers.cpp
../src/details/ArborX_DetailsContainers.hpp(38): warning: __device__ annotation is ignored on a function("StaticVector") that is explicitly defaulted on its first declaration
                                       
../src/details/ArborX_DetailsContainers.hpp(38): warning: __host__ annotation is ignored on a function("StaticVector") that is explicitly defaulted on its first declaration                                                                                                                                    
                          
../src/details/ArborX_DetailsContainers.hpp(38): warning: __device__ annotation is ignored on a function("StaticVector") that is explicitly defaulted on its first declaration                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                                                                                      
../src/details/ArborX_DetailsContainers.hpp(38): warning: __host__ annotation is ignored on a function("StaticVector") that is explicitly defaulted on its first declaration

So, it is likely -Wall -Wextra do not contain that warning. It could also be that warnings flags have to be specified in a different manner for nvcc_wrapper to get passed to nvcc. Maybe using -Xcompiler somehow?

@masterleinad
Copy link
Collaborator

The warning flags nvcc understands is totally different from the usual set for C++ compiler, especially Werror works differently, see https://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#generic-tool-options-Werror.

Suppressing nvcc warnings is really hard most of the times (if it is not explicitly mentioned in the CUDA documentation). For the one here, also have a look at the pull request that introduced KOKKOS_DEFAULTED_FUNCTION kokkos/kokkos#2628.

As a side-note: godbolt has CUDA support to play with these kind of problems https://godbolt.org/z/Q5_mly

@aprokop
Copy link
Contributor Author

aprokop commented Apr 17, 2020

it seems that jenkins warnings plugin does not support nvcc.

@aprokop aprokop marked this pull request as ready for review April 17, 2020 16:53
@aprokop
Copy link
Contributor Author

aprokop commented Apr 17, 2020

retest this please

@aprokop aprokop force-pushed the kokkos-3.1 branch 2 times, most recently from 64df4be to 744950f Compare April 17, 2020 17:45
@aprokop
Copy link
Contributor Author

aprokop commented Apr 17, 2020

OK, please don't breathe on this PR, the checks passed :)
@dalg24 You wanted to push something to it, right?

@dalg24
Copy link
Contributor

dalg24 commented Apr 18, 2020

@dalg24 You wanted to push something to it, right?

I did but you forced push over it...

@aprokop
Copy link
Contributor Author

aprokop commented Apr 18, 2020

@dalg24 I apologize, really should not have done it. Did not realize you were working on it too.

@aprokop
Copy link
Contributor Author

aprokop commented Apr 18, 2020

@dalg24 Can you please approve and merge once the tests pass?

@dalg24 dalg24 merged commit 28b7431 into arborx:master Apr 18, 2020
@aprokop aprokop deleted the kokkos-3.1 branch April 18, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build and installation documentation testing Anything to do with tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants