HIP: CMake work on tests and ETI to truely enable the backend #820

lucbv · 2020-09-30T16:13:44Z

The goal is to make sure the HIP backend in properly instanciated
by ETI and tested in the unit-test using the Kokkos::Experimental::HIP
and Kokkos::Experimental::HIPSpace execution and memory spaces.

ndellingwood

@lucbv Several of the BLAS tests in unit_test/hip have Cuda in the filename and should be renamed with Hip (e.g. unit_test/hip/Test_Cuda_Blas2_gemv.cpp)

lucbv · 2020-10-02T01:37:41Z

@ndellingwood this was meant to be WIP so I can show @jjwilke where I'm at and fix some CMake stuff.
Not all the unit-test are building so I am keeping some commented out, I am thinking of adding some CMake flags to selectively enable/disable some tests at configure time to allow for incremental progress.
But since Kokkos is ahead of me with rock-3.8.0 I think I won't try to push this further until it is installed on caraway.

Once this is done we should be about finished with HIP backend though : )

brian-kelley · 2020-10-06T17:52:17Z

@lucbv @jjwilke I'm working on a branch to add HIP code paths to algorithms, perf tests, examples, etc. For example, limiting vector length to 64 instead of 32. I won't touch anything related to ETI, but when that's done we should be close to having tests pass on caraway.

lucbv · 2020-10-06T20:09:56Z

@brian-kelley just double checking, you are doing that based by working on top of this PR or are you working on top of develop?

brian-kelley · 2020-10-06T20:11:54Z

@lucbv I started out today working from develop, but I can rebase on your branch. Is the plan to only merge HIP work into develop when it all works? If we're ok with having different pieces go in without 100% working, I think this PR could just go in.

(by that, I mean that adding files that only compile with HIP enabled shouldn't break anyone's build)

lucbv · 2020-10-06T20:50:32Z

@brian-kelley
I am debating the idea of merging this but I think it would make sense to see if we can get the PR by @jjwilke first and then rebase this PR on top of it so we can selectively enable tests for HIP builds.
Btw I think that develop builds and runs just fine on caraway right now, but if you grab the HEAD of this branch it will be a different story...
Do you want to meet and talk about this work?

brian-kelley · 2020-10-07T01:16:02Z

@lucbv I will just finish the branch I have now, then when Jeremy's ETI stuff goes in, we can combine our stuff and rebase on that. We could talk about this tomorrow. Today I got through batched, blas, graph, common (sparse was going to be the trickiest).

lucbv · 2020-10-20T23:26:52Z

@ndellingwood this version should be much closer to the final one.
I would like to try to build it against the current develop branch of kokkos and see how much farther it gets, some improvements were made since they upgraded to rocm/3.8.0

unit_test/hip/Test_HIP_Blas1_team_axpby.cpp

unit_test/hip/Test_HIP_Blas1_team_axpy.cpp

unit_test/hip/Test_HIP_Blas1_team_dot.cpp

unit_test/hip/Test_HIP_Blas1_team_mult.cpp

unit_test/hip/Test_HIP_Blas1_team_nrm2.cpp

unit_test/hip/Test_HIP_Blas1_team_scal.cpp

unit_test/hip/Test_HIP_Blas1_team_update.cpp

unit_test/hip/Test_HIP_Blas2_team_gemv.cpp

unit_test/hip/Test_HIP_Blas1_team_abs.cpp

lucbv · 2020-10-21T13:05:30Z

@ndellingwood thanks for the look, it's actually a good thing, I just used a script to automatically get all the Cuda unit-test into the HIP folder and then changed the header inclusion. Maybe I should actually do a grep for Cuda in the HIP unit-test folder just to see what comes out of it?
I'll address your comments : )

lucbv · 2020-10-29T15:31:20Z

@srajama1 @ndellingwood @brian-kelley @jjwilke
This work is ready for review, I have started a spot_check on Kokkos-dev2 that could reveal something new after changes I made to the unit-tests this morning (spot-check I ran last night was clean).
The main thing left to make this completely clean will be to apply the changes that @jjwilke has worked on in PR #835 that will allow me to uncomment some tests that are currently not building.

Regarding what works and what does not after this PR:

The library builds on caraway (node 04 with rocm/3.8.0 at least)
The ETI is in place and instantiate the library correctly on Kokkos::Experimental::HIP and Kokkos::Experimental::HIPSpace
All the perf_tests are building correctly
The Blas and Sparse tests are building correctly, other tests will be enabled at a later time
Overall both Blas and Sparse unit-tests are failing but in detail a good chunk of sub tests in gtest are passing, some work to clean the rest will be needed
The wiki tests are passing but the default types used there might not be appropriate, need to double check. -> actually checked and @brian-kelley modified that correctly so it runs on HIP
Running the perf_tests was not attempted yet.

One thing to do after this PR will be to modify the spot-check so that it can run on caraway. While it cannot check for passing tests, just checking for compilation would already be helpful. Especially so since rocm/3.8.0 is using clang-11 as compiler and we do not test with it (currently kokkos-dev2 tests with clang 8.0 and clang 9.0.0).

Please let me know if this makes sense or if you have a strong opinion like: Blas or Sparse should pass before merging?

brian-kelley · 2020-10-29T16:30:42Z

I would say, as long as it doesn't break serial/cuda/openmp we should just merge anything that gets closer to HIP working. It's easier than having several diverging branches of develop with the updates. We just shouldn't advertise to users that they should try hip yet :)

For question 6 you had, I just double checked in all the wiki examples that they are calling HIP, which is the DefaultExecutionSpace in a HIP+Serial build.

For perf tests, all the graph ones seem to work completely. I just tried several of the D1 and D2 coloring algorithms on some pretty big graphs, and mis2 also works. Coloring is producing a plausible color array and mis2 can verify the output. I did miss the D1 coloring when adding HIP support but I have a PR #841 for that.

lucbv · 2020-10-29T16:45:43Z

Yeah that's my philosophy too, let's see what others say.
Also here are the results from kokkos-dev2 spot-check:

#######################################################
PASSED TESTS
#######################################################
clang-8.0-Cuda_OpenMP-release build_time=692 run_time=147
clang-8.0-Pthread_Serial-release build_time=235 run_time=131
clang-9.0.0-Pthread-release build_time=150 run_time=62
clang-9.0.0-Serial-release build_time=160 run_time=48
cuda-10.1-Cuda_OpenMP-release build_time=838 run_time=146
cuda-11.0-Cuda_OpenMP-release build_time=895 run_time=308
cuda-9.2-Cuda_Serial-release build_time=813 run_time=215
gcc-7.3.0-OpenMP-release build_time=166 run_time=47
gcc-7.3.0-Pthread-release build_time=130 run_time=62
gcc-8.3.0-Serial-release build_time=159 run_time=50
gcc-9.1-OpenMP-release build_time=206 run_time=47
gcc-9.1-Serial-release build_time=175 run_time=55
intel-17.0.1-Serial-release build_time=404 run_time=52
intel-18.0.5-OpenMP-release build_time=681 run_time=49
intel-19.0.5-Pthread-release build_time=357 run_time=64

unit_test/hip/Test_HIP.hpp

unit_test/CMakeLists.txt

brian-kelley

The HIP device should be HIP, not HIPSpace (see 2 comments)

lucbv · 2020-10-29T18:47:42Z

@brian-kelley I find the exec space issue and enable common and graph tests!

by ETI and tested in the unit-test using the Kokkos::Experimental::HIP and Kokkos::Experimental::HIPSpace execution and memory spaces. Status of the unit-tests: Batched -> does not build Blas -> builds and Blas1 test pass, others fail Common -> does not build Graph -> does not build Sparse -> builds but does few tests run before crash Fix blas and blas3 perf_test: these perf_test use parallel_for to run multiple tests at once while on host and call a host only function to do so. These are now being skipped when KOKKOS_ENABLE_HIP is on. All performance test are building.

TestExecSpace was set to HIPSpace instead of HIP creating problems at build and execution time in unit_test. We can now enable common and graph unit-tests for build. common_hip is passing without failures.

The min launch bound was set to 2 and is now set to 0. This new setting allows the BLAS unit-test to successfully run to completion.

With the new modifications the spot-check can be run on caraway04 where BLAS and Common unit-tests will be built and tested as well as the wiki tests.

lucbv · 2020-10-30T13:38:57Z

With a bit more work done in that last couple commits we now have the spot-check building and testing BLAS and Common unit-tests and wiki examples successfully on caraway04.

caraway04 spot-check

#######################################################
PASSED TESTS
#######################################################
rocm-3.8.0-Hip_Serial-release build_time=667 run_time=121

kokkos-dev2 spot-check

#######################################################
PASSED TESTS
#######################################################
clang-8.0-Cuda_OpenMP-release build_time=740 run_time=733
clang-8.0-Pthread_Serial-release build_time=270 run_time=144
clang-9.0.0-Pthread-release build_time=165 run_time=71
clang-9.0.0-Serial-release build_time=160 run_time=48
cuda-10.1-Cuda_OpenMP-release build_time=876 run_time=149
cuda-11.0-Cuda_OpenMP-release build_time=906 run_time=153
cuda-9.2-Cuda_Serial-release build_time=856 run_time=228
gcc-7.3.0-OpenMP-release build_time=164 run_time=48
gcc-7.3.0-Pthread-release build_time=132 run_time=64
gcc-8.3.0-Serial-release build_time=163 run_time=50
gcc-9.1-OpenMP-release build_time=207 run_time=49
gcc-9.1-Serial-release build_time=199 run_time=51
intel-17.0.1-Serial-release build_time=393 run_time=54
intel-18.0.5-OpenMP-release build_time=701 run_time=51
intel-19.0.5-Pthread-release build_time=391 run_time=72

srajama1 · 2020-10-31T18:18:13Z

All , @lucbv @brian-kelley @jjwilke : First, thanks for all the progress on HIP backend. Let me review the actual changes next. In principle I would like incremental changes over one massive change. As long as we can clearly identify the failures (turn them off) create an issue with all of them so we can make progress on those, we can push this to develop. I do not want to advertise HIP support unless we have reached a point we have clean tests (except when there are external issues from TPLs or compilers and we can disable tests for those). On to code now.

srajama1

Code wise looks ok. My major comments :

please file issues or one master issue for all the problems even if there is a workaround now.
can we avoid 100+ files for every backend ?

srajama1 · 2020-10-31T18:22:09Z

perf_test/blas/blas3/KokkosBlas3_trmm_perf_test.hpp

@@ -292,7 +292,7 @@ void __do_trmm_serial_batched(options_t options, trmm_args_t trmm_args) {
  return;
 }

-#if !defined(KOKKOS_ENABLE_CUDA)
+#if !defined(KOKKOS_ENABLE_CUDA) && !defined(KOKKOS_ENABLE_HIP)


Are these basically "host only" checks ? Then we can add one check as host only and not worry about this for Intel SYCL and OpenMP target backends.

Good point, we can definitely create a KOKKOSKERNELS_HOST_ONLY macro that stores that information and is used in the code.

see issue #843

srajama1 · 2020-10-31T18:23:18Z

src/blas/impl/KokkosBlas3_gemm_impl.hpp

+    // yield an optimal_num_blocks=0 which means no ressources
+    // are allocated... Switching to LaunchBounds<384,2> fixes
+    // that problem but I'm not sure if that it a good perf
+    // parameter or why it is set to 2 for Cuda?


File a Kokkos issue ?

Yeah, I'll talk about it with Damien and Daniel, some parameters are inherently different with AMD like the wrap size so this could be something that needs to be different depending on the architecture?

srajama1 · 2020-10-31T18:24:24Z

unit_test/CMakeLists.txt

+  #   COMPONENTS graph
+  # )
+
+  #currently float 128 test is not working. So common tests are explicitly added.  


This need to be in list of issues to fix.

@srajama1 Note that float128 tests are also diabled for CUDA, so it's generally just not supported now on GPU. I think because of that this should be lower priority than the other tests

Ok, got it.

srajama1 · 2020-10-31T18:27:17Z

unit_test/hip/Test_HIP_Batched_SerialEigendecomposition_Real.cpp

@@ -0,0 +1,3 @@
+#include "Test_HIP.hpp"


All these test files needed ? I am worried we are going in the direction of the ETI again. This is going to blow up with OpenMP target and Intel SYCL. Can we do same trick as we did for ETI, where we generate these files if unit tests are ON ?

Yes something like that or having all the headers for the different backends in a single file and use guards to enable them selectively at compile time with different executably names.

see issue #845

rsiva-web · 2020-11-02T20:48:17Z

Can I merge this with the assumption above comments will be filed as an issue ? It would be nice for this to go in the next release.

lucbv · 2020-11-02T21:42:15Z

Yes feel free to merge, I am filling new issues for the points raised here.

lucbv added the enhancement label Sep 30, 2020

lucbv requested a review from jjwilke September 30, 2020 16:13

lucbv self-assigned this Sep 30, 2020

lucbv mentioned this pull request Sep 30, 2020

HIP backend general issue #806

Closed

ndellingwood reviewed Sep 30, 2020

View reviewed changes

lucbv force-pushed the HIP_CMake_work branch 2 times, most recently from 569fdd0 to 1f0e85a Compare October 1, 2020 23:51

lucbv force-pushed the HIP_CMake_work branch from 1f0e85a to 3214a88 Compare October 20, 2020 23:22

ndellingwood reviewed Oct 21, 2020

View reviewed changes

lucbv force-pushed the HIP_CMake_work branch 2 times, most recently from d8229bd to 7bdfe10 Compare October 29, 2020 03:57

brian-kelley reviewed Oct 29, 2020

View reviewed changes

unit_test/hip/Test_HIP.hpp Outdated Show resolved Hide resolved

brian-kelley reviewed Oct 29, 2020

View reviewed changes

unit_test/CMakeLists.txt Show resolved Hide resolved

brian-kelley requested changes Oct 29, 2020

View reviewed changes

lucbv force-pushed the HIP_CMake_work branch from 82efdab to 42e9c9a Compare October 29, 2020 18:47

brian-kelley approved these changes Oct 29, 2020

View reviewed changes

brian-kelley mentioned this pull request Oct 29, 2020

Update testall #842

Merged

lucbv added 3 commits October 29, 2020 23:35

HIP: fixing bug in Test_HIP.hpp and enabling more tests

8d6ce5f

TestExecSpace was set to HIPSpace instead of HIP creating problems at build and execution time in unit_test. We can now enable common and graph unit-tests for build. common_hip is passing without failures.

HIP: removing unecessary CUDA guard from HIP unit-tests

6206bfe

HIP: fixing gemm unit-test by changing kernel launch bounds

a1afb71

The min launch bound was set to 2 and is now set to 0. This new setting allows the BLAS unit-test to successfully run to completion.

lucbv force-pushed the HIP_CMake_work branch from 42e9c9a to a1afb71 Compare October 30, 2020 05:36

HIP: modifying unit_test/CMakeList and cm_test_all_sandia

f06aaa5

With the new modifications the spot-check can be run on caraway04 where BLAS and Common unit-tests will be built and tested as well as the wiki tests.

srajama1 approved these changes Oct 31, 2020

View reviewed changes

srajama1 merged commit 9e338ef into kokkos:develop Nov 3, 2020

lucbv deleted the HIP_CMake_work branch November 3, 2020 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIP: CMake work on tests and ETI to truely enable the backend #820

HIP: CMake work on tests and ETI to truely enable the backend #820

lucbv commented Sep 30, 2020

ndellingwood left a comment

lucbv commented Oct 2, 2020

brian-kelley commented Oct 6, 2020

lucbv commented Oct 6, 2020

brian-kelley commented Oct 6, 2020 •

edited

Loading

lucbv commented Oct 6, 2020

brian-kelley commented Oct 7, 2020

lucbv commented Oct 20, 2020

lucbv commented Oct 21, 2020

lucbv commented Oct 29, 2020 •

edited

Loading

brian-kelley commented Oct 29, 2020

lucbv commented Oct 29, 2020 •

edited

Loading

brian-kelley left a comment

lucbv commented Oct 29, 2020

lucbv commented Oct 30, 2020

srajama1 commented Oct 31, 2020

srajama1 left a comment

srajama1 Oct 31, 2020

lucbv Nov 1, 2020

lucbv Nov 3, 2020

srajama1 Oct 31, 2020

lucbv Nov 1, 2020

srajama1 Oct 31, 2020

brian-kelley Nov 2, 2020

rsiva-web Nov 2, 2020

srajama1 Oct 31, 2020

lucbv Nov 1, 2020

lucbv Nov 3, 2020

rsiva-web commented Nov 2, 2020

lucbv commented Nov 2, 2020

HIP: CMake work on tests and ETI to truely enable the backend #820

HIP: CMake work on tests and ETI to truely enable the backend #820

Conversation

lucbv commented Sep 30, 2020

ndellingwood left a comment

Choose a reason for hiding this comment

lucbv commented Oct 2, 2020

brian-kelley commented Oct 6, 2020

lucbv commented Oct 6, 2020

brian-kelley commented Oct 6, 2020 • edited Loading

lucbv commented Oct 6, 2020

brian-kelley commented Oct 7, 2020

lucbv commented Oct 20, 2020

lucbv commented Oct 21, 2020

lucbv commented Oct 29, 2020 • edited Loading

brian-kelley commented Oct 29, 2020

lucbv commented Oct 29, 2020 • edited Loading

brian-kelley left a comment

Choose a reason for hiding this comment

lucbv commented Oct 29, 2020

lucbv commented Oct 30, 2020

caraway04 spot-check

kokkos-dev2 spot-check

srajama1 commented Oct 31, 2020

srajama1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsiva-web commented Nov 2, 2020

lucbv commented Nov 2, 2020

brian-kelley commented Oct 6, 2020 •

edited

Loading

lucbv commented Oct 29, 2020 •

edited

Loading

lucbv commented Oct 29, 2020 •

edited

Loading