jsitaraman / tioga

Tioga is a library for overset grid assembly on parallel distributed systems
GNU Lesser General Public License v3.0
64 stars 36 forks source link

Add ArborX #47

Closed aprokop closed 3 years ago

aprokop commented 3 years ago

This is a draft PR for now. For the most part, the PR is straightforward. However, I found that I had to disable LINKER_LANGUAGE on Summit to be able to build the executables.

aprokop commented 3 years ago

OK, here's the current status. I tested this PR with tioga-utils on Summit. As an example problem, I ran several iterations of rotating turbine:

For validation, I printed donorId[i] from one of the ranks for 2 iterations. ADT and ArborX matched exactly.

Performance comparison was done in Serial (left ADT, right ArborX): Screen Shot 2021-01-20 at 1 48 16 PM indicating that the overall time went down for this simulation by about 25%.

I also printed out individual construction and search timers from each rank (using MPI_Wtime() as timers), and plotted against iteration. Here are a few ranks results: Rank 0 mpi-0 Rank 2 mpi-2 Rank 18 mpi-18 Full collection is attached: mpi-ranks.zip

I'd like to get some feedback and understanding on what directions to pursue further. Can this be merged as it is? Is there anything else required? Am I fundamentally working on the correct thing, or is there another codebase that I'm missing? So far, I have not seen and GPU search work in the exawind branch, which I think I should be using.

One thing is that I'm not currently able to compile TIOGA without using exawind-builder. Not sure why that is.

aprokop commented 3 years ago

So the issue (at least in the HIP build) seems to stem from -D-pthread option in the command line

cd /home/runner/work/tioga/tioga/build/driver && /usr/bin/f95 -D-pthread -DTIOGA_HAS_GPU -DTIOGA_HAS_HIP -DTIOGA_HAS_NODEGID -I/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi -I/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/opal/mca/event/libevent2022/libevent -I/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/opal/mca/event/libevent2022/libevent/include -I/usr/lib/x86_64-linux-gnu/openmpi/include -I/usr/lib/x86_64-linux-gnu/openmpi/lib -I/home/runner/work/tioga/tioga/src -fbounds-check -fbacktrace -fdefault-real-8 -ffree-line-length-none -cpp -c /home/runner/work/tioga/tioga/driver/testTioga.f90 -o CMakeFiles/tioga.exe.dir/testTioga.f90.o

This may be the result of disabling LINKER_LANGUAGE Fortran for executables in this PR. However, if I reenable it, I believe building executables will fail on Summit. So, it may be that enabling ArborX would require disabling executables builds, unless an alternative approach is found.

aprokop commented 3 years ago

@sayerhs Yes, you were absolutely right, it was as easy as f90 -> F90 change. So the CI passes. However, the LINKER_LANGUAGE thing still prevents properly building ArborX:

gfortran: error: unrecognized command line option '--relocatable-device-code=true'
gfortran: error: unrecognized command line option '-arch=sm_70'

It does pass the CI with LINKER_LANGUAGE commented out, so maybe it's fine to completely remove it.

I think it's fine in this form, I see no outstanding issues. It's already rebased on the latest exawind branch, and should be ready for merge.

sayerhs commented 3 years ago

@jsitaraman This is ready to merge. Is there anything else you need to see before we can merge this?

aprokop commented 3 years ago

@jsitaraman ping