Remove FindHIP and use hip-config.cmake instead #526

davidbeckingsale · 2021-09-17T20:30:33Z

No description provided.

trws · 2021-09-22T04:03:34Z

This seems like a substantial improvement. A breaking change, but a substantial improvement nevertheless.

davidbeckingsale · 2021-09-22T15:05:13Z

Yup, two major changes:

no hip-> nvcc support.
new target names blt::hip and blt::hip_runtime

I don't think people were using hip->nvcc anyway, and we can revisit that with the builtin CMake HIP support.
For the new target names, it's not ideal but there is already a hip target created by the hip-config so I was unable to use that name. The hip_runtime target could remain but I renamed it for consistency.

kennyweiss

Not a review (yet), but please update the RELEASE-NOTES with this change, especially since it seems like a breaking change.

Some other questions:

does this require a specific version of CMake ?
will this PR necessitate a new BLT release?

trws · 2021-09-24T19:16:24Z

It probably requires a specific version of rocm, but at least from my testing 3.14+ works just fine in cmake. I don't tend to keep any lower around so I can't speak to that.

davidbeckingsale · 2021-09-24T21:54:41Z

Tom's right, just a specific HIP/rocm version, but I have checked back a long way and the file was there, so I think we should be good to go.

The change is only breaking due to target names so if there was a way to avoid that I think it would be preferable. Happy to take suggestions. If not, I think a release would be appropriate to make it easy to do things like Spack packages (things would have to be conditional based on BLT version).

trws · 2021-09-25T22:16:15Z

I personally think it would be worth it, but maybe lumped in with a rework so cuda can be used the same way? Or as an experimental option under a flag until it can be consistent? FWIW I actually really like the blt::<target> convention. It makes it more clear what’s going on, and makes it more discoverable with searches and completion for example.

davidbeckingsale · 2021-10-29T00:00:09Z

Hey folks I would be interested in getting this merged. Kenny mentioned updating release notes, but I think the fact that this requires namespacing the targets requires some more discussion. I'm happy to add the namespaces (and alias) to the other imported BLT targets for consistency, and we can deprecate the non-namespaced versions.

Thoughts?

kennyweiss

This does seem like a really nice cleanup of the hip setup and logic.

I like the blt::hip namespace change
For consistency, we should probably do the same to our other similar targets, e.g. nvcc, mpi, openmp, ...
- I agree that we should think about/discuss these changes
Since this is a breaking change, we should also consider creating a new release.
In the meantime, it should definitely be documented in our RELEASE-NOTES

Since I have less familiarity with some of the implications, I'll wait on others to approve this before me.

Edit: Clarifying my second bullet -- if we are going to require people to update their build systems to accommodate the blt::hip namespace change, and are thinking about doing similar things for other dependencies, like nvcc, it would probably be a good idea to do it all at once.

kennyweiss · 2021-10-29T00:31:12Z

cmake/BLTOptions.cmake

@@ -59,9 +59,6 @@ mark_as_advanced(ENABLE_CLANG_CUDA)
 set(BLT_CLANG_CUDA_ARCH "sm_30" CACHE STRING "Compute architecture to use when generating CUDA code with Clang")
 mark_as_advanced(BLT_CLANG_CUDA_ARCH)
 option(ENABLE_HIP         "Enable HIP support" OFF)
-cmake_dependent_option(ENABLE_CLANG_HIP   "Enable Clang's native HIP support" OFF


Can you please comment on why ENABLE_CLANG_HIP is no longer necessary?

Bonus: It seems that we still need ENABLE_CLANG_CUDA in some cuda cases. Please also comment on that.
It would be nice if we could document some of this, perhaps in a follow up PR.

tests/smoke/CMakeLists.txt

kennyweiss · 2021-10-29T02:37:21Z

I tried testing this branch locally on rznevada using cmake@3.19.2
and got some compilation warnings and errors (in addition to the missing blt::hip_runtime that I commented on above):

Config and build lines

> ml cmake/3.19.2 
> mkdir build-debug && cd build-debug
> cmake -C ../host-configs/llnl/toss_4_x86_64_ib_cray/cce@12.0.0_clang_hip.cmake ../tests/internal/
> make

Warning (seems safe to ignore)

<blt>/tests/internal/src/static_analysis/subtle_error_source.cpp:23:23: warning: array index 1 is past the end of the array (which contains 1 element) [-Warray-bounds]
  int first_element = array[1];
                      ^     ~
<blt>/tests/internal/src/static_analysis/subtle_error_source.cpp:22:3: note: array 'array' declared here
  int array[1] = {5};
  ^

Linker error

[ 59%] Linking CXX executable ../../../tests/blt_hip_smoke
/opt/cray/pe/cce/12.0.0/cce/x86_64/bin/cce_omp_offload_linker: Unsupported hip accelerator target: gfx900
clang-12: error: linker command failed with exit code 255 (use -v to see invocation)
make[2]: *** [blt/tests/smoke/CMakeFiles/blt_hip_smoke.dir/build.make:104: tests/blt_hip_smoke] Error 255
make[1]: *** [CMakeFiles/Makefile2:1978: blt/tests/smoke/CMakeFiles/blt_hip_smoke.dir/all] Error 2

Edit: Are you seeing these, or am I doing something wrong?

Update: Looks like the warning is related to a static_analysis test and is expected, so we can ignore that one.

Update 2: I'm seeing the same error with cmake@3.21.1.
Even though the host config is explicitly specifying set(CMAKE_HIP_ARCHITECTURES "gfx908" CACHE STRING "")
Something is injecting lots of -offload-arch flags:
--offload-arch=gfx900 --offload-arch=gfx906 --offload-arch=gfx908

Here's the full build and link line from cmake/make

[ 31%] Building CXX object blt/tests/smoke/CMakeFiles/blt_hip_gtest_smoke.dir/blt_hip_gtest_smoke.cpp.o
cd <blt>/build-debug/blt/tests/smoke && /usr/tce/packages/cce-tce/cce-12.0.0/bin/crayCC
	-DGTEST_HAS_DEATH_TEST=0
	-D__HIP_PLATFORM_AMD__=1
	-D__HIP_PLATFORM_HCC__=1
	-I<blt>/thirdparty_builtin/googletest-master-2020-01-07/googletest
	-isystem <blt>/thirdparty_builtin/googletest-master-2020-01-07/googletest/include
	-isystem /opt/rocm-4.2.0/hip/../include
	-isystem /opt/rocm-4.2.0/llvm/lib/clang/12.0.0/include/..
	-isystem /opt/rocm-4.2.0/hip/include
	-isystem /opt/rocm-4.2.0/include
	-Wall
	-Wextra     
	-fPIE
	--rocm-path=/opt/rocm-4.2.0
	-xhip
	--offload-arch=gfx900
	--offload-arch=gfx906
	--offload-arch=gfx908
	-std=c++11
	-MD
	-MT blt/tests/smoke/CMakeFiles/blt_hip_gtest_smoke.dir/blt_hip_gtest_smoke.cpp.o
	-MF CMakeFiles/blt_hip_gtest_smoke.dir/blt_hip_gtest_smoke.cpp.o.d
	-o CMakeFiles/blt_hip_gtest_smoke.dir/blt_hip_gtest_smoke.cpp.o
	-c <blt>/tests/smoke/blt_hip_gtest_smoke.cpp

[ 33%] Linking CXX executable ../../../tests/blt_hip_gtest_smoke
cd <blt>/build-debug/blt/tests/smoke && /usr/tce/backend/installations/linux-rhel8-x86_64/gcc-10.2.1/cmake-3.21.1-r36vais2fcnwnbsd63glpwypg55bts7y/bin/cmake
	-E cmake_link_script CMakeFiles/blt_hip_gtest_smoke.dir/link.txt
	--verbose=1
/usr/tce/packages/cce-tce/cce-12.0.0/bin/crayCC 
	-Wall
	-Wextra      CMakeFiles/blt_hip_gtest_smoke.dir/blt_hip_gtest_smoke.cpp.o
	-o ../../../tests/blt_hip_gtest_smoke 
	-Wl,-rpath,/opt/rocm-4.2.0/hip/lib:/opt/rocm-4.2.0/lib ../../../lib/libgtest_main.a ../../../lib/libgtest.a /opt/rocm-4.2.0/hip/lib/libamdhip64.so.4.2.40200
	--hip-link
	--offload-arch=gfx900
	--offload-arch=gfx906
	--offload-arch=gfx908
	-L"/opt/rocm-4.2.0/llvm/lib/clang/12.0.0/include/../lib/linux"
	-lclang_rt.builtins-x86_64
	-Wl,-rpath-link,/opt/rocm-4.2.0/lib 

/opt/cray/pe/cce/12.0.0/cce/x86_64/bin/cce_omp_offload_linker: Unsupported hip accelerator target: gfx900

clang-12: error: linker command failed with exit code 255 (use
	-v to see invocation)
make[2]: *** [blt/tests/smoke/CMakeFiles/blt_hip_gtest_smoke.dir/build.make:100: tests/blt_hip_gtest_smoke] Error 255

kennyweiss · 2021-10-29T03:11:25Z

host-configs/llnl/toss_4_x86_64_ib_cray/cce@12.0.0_clang_hip.cmake


 set(ROCM_PATH "/opt/rocm-4.1.0/" CACHE PATH "")
-set(HIP_PLATFORM "clang" CACHE STRIING "")

 set(BLT_CLANG_HIP_ARCH "gfx908" CACHE STRING "")


BLT_CLANG_HIP_ARCH got removed in #521
I think it should be:

Suggested change

set(BLT_CLANG_HIP_ARCH "gfx908" CACHE STRING "")

set(CMAKE_HIP_ARCHITECTURES "gfx908" CACHE STRING "")

kennyweiss · 2021-10-29T04:38:57Z

@davidbeckingsale -- I discovered that our current hip host-config was also broken and created #537 to fix it for cmake@3.19. It's still not working correctly for cmake@3.21.

See also #536

davidbeckingsale · 2021-11-01T22:30:38Z

Thanks @kennyweiss for taking a look at this. I'll make the changes you have suggested and will investigate the build issue - I have a hunch as to where it might be coming from.

Patch hip::device target to allow compilation/running without FindHIP

DavidPoliakoff · 2021-11-09T23:28:54Z

Just FYI, this fixes a really obnoxious bug for us in "Kokkos + Perfsuite interoperability" land. So if it hits credibility on your side, we'd love to see a merge (and ideally then a merge of that BLT into RAJA, and that RAJA into Perfsuite, if you're feeling charitable :) )

rhornung67 · 2021-11-09T23:47:06Z

Just FYI, this fixes a really obnoxious bug for us in "Kokkos + Perfsuite interoperability" land. So if it hits credibility on your side, we'd love to see a merge (and ideally then a merge of that BLT into RAJA, and that RAJA into Perfsuite, if you're feeling charitable :) )

@DavidPoliakoff that's the plan. a few folks are out on vacation until the middle of next week. we'll work on a perf suite release after that. Actually, it looks like the next BLT release will not come out until after the new year. So it will be a while before this gets pulled into RAJA repos.

white238 · 2021-11-10T18:57:30Z

cmake/thirdparty/SetupHIP.cmake

-                       DEFINES          ${_hip_compile_defines}
-                       COMPILE_FLAGS    ${_hip_compile_flags})
+if (NOT ROCM_PATH)
+find_path(ROCM_PATH


bmhan12 · 2021-11-11T00:24:20Z

I'm encountering the following errors during configuration when using this branch in the blt submodule for axom:

CMake Error at blt/cmake/BLTMacros.cmake:559 (add_library):
  The target name "blt::hip" is reserved or not valid for certain CMake
  features, such as generator expressions, and may result in undefined
  behavior.
Call Stack (most recent call first):
  blt/cmake/thirdparty/SetupHIP.cmake:27 (blt_import_library)
  blt/cmake/thirdparty/SetupThirdParty.cmake:56 (include)
  blt/SetupBLT.cmake:112 (include)
  CMakeLists.txt:167 (include)


CMake Error at blt/cmake/BLTMacros.cmake:437 (message):
  blt_patch_target() NAME argument must be a native CMake target
Call Stack (most recent call first):
  blt/cmake/BLTMacros.cmake:566 (blt_patch_target)
  blt/cmake/thirdparty/SetupHIP.cmake:27 (blt_import_library)
  blt/cmake/thirdparty/SetupThirdParty.cmake:56 (include)
  blt/SetupBLT.cmake:112 (include)
  CMakeLists.txt:167 (include)

trws · 2021-11-16T14:51:56Z

That error is peculiar, what version of cmake are you using?

davidbeckingsale · 2021-11-16T14:58:38Z

That error is peculiar, what version of cmake are you using?

Lots of us have hit this - a target that is not imported cannot have :: in the name https://cmake.org/cmake/help/latest/policy/CMP0028.html

BLT provides an option to export the tpl targets (meaning they can't be created as imported libraries) - this error appears when the option BLT_EXPORT_THIRDPARTY is On.

trws · 2021-11-16T15:14:38Z

Imported or ALIAS, can we name the target __blt_dont_use_this_one_seriously_i_mean_it_hip and make an alias to it that's blt::hip?

davidbeckingsale · 2021-11-16T15:16:33Z

Imported or ALIAS, can we name the target __blt_dont_use_this_one_seriously_i_mean_it_hip and make an alias to it that's blt::hip?

Great idea! I think that should work.

cyrush · 2021-12-09T23:58:11Z

For the export targets issue:

When used downstream, aren't projects exporting blt's targets into their own namespaces anyway?
Like:

https://github.com/LLNL/Umpire/blob/e118325fa68bda4a1cb5510e825e6e8680525353/cmake/SetupUmpireThirdParty.cmake#L72

If we called these blt_zzz that would be an improvement and still compatible with this strategy?

They would be namespaces, but protected in the scope of the imported project.

white238 · 2022-01-10T18:43:57Z

This is good to merge pending someone else's review and my couple changes.

Also please update the release notes and this page docs/tutorial/common_hpc_dependencies.rst. You don't have to go all out in documenting the latter but please update the target names.

Thanks @davidbeckingsale !

davidbeckingsale · 2022-01-10T19:07:21Z

Also please update the release notes and this page docs/tutorial/common_hpc_dependencies.rst. You don't have to go all out in documenting the latter but please update the target names.

Good catch, will do.

cmake/thirdparty/SetupHIP.cmake

Co-authored-by: Chris White <white238@llnl.gov>

kennyweiss

Thanks for working through this @davidbeckingsale !

kennyweiss · 2022-01-10T21:13:17Z

docs/tutorial/exporting_targets.rst

+.. note:: The recommended usage of the HIP targets is via the ``blt::hip`` and
+    ``blt::hip_runtime`` aliases. Alias targets cannot be exported, so the
+    ``blt_hip``/``blt_hip_runtime`` names can be used for this purpose.
+


Should we mention anything about the underlying targets, and if those are still available for users?
(I think those are hip::host and hip::device, but I'm not sure)

They are available, but hip::host actually seems broken (missing include) so I don't think we should recommend it.

The behavior would also be different would it not?

I admit I'm wondering, especially with this doc change, if having the aliases is worth it since now there's a split between at least two uses. Should it just always be blt_<backend> perhaps rather than have both? At least that way it can be made consistent everywhere. Really not fond of how aliases only kinda-sorta work across projects like this.

😭

Installing a target provided by BLT is a pretty specific use case that afaik only a couple of projects are doing. Using blt_<backend> is fine but just looks wrong to me, I feel like the namespaced target is correct since we want people to think of them as imported targets.

Co-authored-by: Chris White <white238@llnl.gov>

Remove FindHIP and use hip-config.cmake instead

db5667e

davidbeckingsale requested review from white238, trws and joshessman-llnl September 17, 2021 20:30

Merge branch 'develop' into feature/no-findhip

c4d6a8f

kennyweiss reviewed Sep 24, 2021

View reviewed changes

kennyweiss reviewed Oct 29, 2021

View reviewed changes

tests/smoke/CMakeLists.txt Show resolved Hide resolved

kennyweiss reviewed Oct 29, 2021

View reviewed changes

kennyweiss mentioned this pull request Oct 29, 2021

hip_smoke_tests broken with cmake@3.21 #536

Closed

kennyweiss mentioned this pull request Oct 29, 2021

Updates hip cce host-config #537

Merged

joshessman-llnl and others added 8 commits November 3, 2021 12:43

fix: patch hip::device

2c4de7d

Merge branch 'bugfix/kweiss/hip_smoke_test' into bugfix/patch-hip-device

5b5ada4

build: add newer rzwhamo hostconfig

555c1b7

Merge branch 'develop' into feature/no-findhip

f40257e

Merge branch 'feature/no-findhip' into bugfix/patch-hip-device

aba82d6

docs: apply suggestions from @kennyweiss

fb293b8

Merge pull request #541 from LLNL/bugfix/patch-hip-device

75a48a4

Patch hip::device target to allow compilation/running without FindHIP

Merge branch 'develop' into feature/no-findhip

d144901

white238 reviewed Nov 10, 2021

View reviewed changes

Merge branch 'develop' into feature/no-findhip

08babb7

adj hip target name in smoke test

fd0604a

davidbeckingsale added 3 commits January 6, 2022 14:14

Use inherit_target_info and aliases

3c7ceb1

Fix target naming in tests

032f836

Use nicer name for internal target

28560bf

davidbeckingsale added 2 commits January 10, 2022 11:36

Add include to blt::hip_runtime

17d73eb

Update tutorial and release notes

e945bed

white238 requested changes Jan 10, 2022

View reviewed changes

cmake/thirdparty/SetupHIP.cmake Outdated Show resolved Hide resolved

cmake/thirdparty/SetupHIP.cmake Show resolved Hide resolved

Fixup ENV usage.

293c48b

Co-authored-by: Chris White <white238@llnl.gov>

white238 approved these changes Jan 10, 2022

View reviewed changes

kennyweiss approved these changes Jan 10, 2022

View reviewed changes

Update cmake/thirdparty/SetupHIP.cmake

2e409a8

Co-authored-by: Chris White <white238@llnl.gov>

white238 mentioned this pull request Jan 11, 2022

Create macro to help export BLT targets #552

Closed

davidbeckingsale merged commit 667158f into develop Jan 11, 2022

white238 deleted the feature/no-findhip branch January 11, 2022 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove FindHIP and use hip-config.cmake instead #526

Remove FindHIP and use hip-config.cmake instead #526

davidbeckingsale commented Sep 17, 2021

trws commented Sep 22, 2021

davidbeckingsale commented Sep 22, 2021

kennyweiss left a comment

trws commented Sep 24, 2021

davidbeckingsale commented Sep 24, 2021

trws commented Sep 25, 2021 via email

davidbeckingsale commented Oct 29, 2021

kennyweiss left a comment •

edited

Loading

kennyweiss Oct 29, 2021

kennyweiss commented Oct 29, 2021 •

edited

Loading

kennyweiss Oct 29, 2021

kennyweiss commented Oct 29, 2021

davidbeckingsale commented Nov 1, 2021

DavidPoliakoff commented Nov 9, 2021

rhornung67 commented Nov 9, 2021 •

edited

Loading

white238 Nov 10, 2021

bmhan12 commented Nov 11, 2021

trws commented Nov 16, 2021

davidbeckingsale commented Nov 16, 2021

trws commented Nov 16, 2021

davidbeckingsale commented Nov 16, 2021

cyrush commented Dec 9, 2021

white238 commented Jan 10, 2022 •

edited

Loading

davidbeckingsale commented Jan 10, 2022

kennyweiss left a comment

kennyweiss Jan 10, 2022

davidbeckingsale Jan 10, 2022

trws Jan 10, 2022

davidbeckingsale Jan 10, 2022

	set(BLT_CLANG_HIP_ARCH "gfx908" CACHE STRING "")
	set(CMAKE_HIP_ARCHITECTURES "gfx908" CACHE STRING "")

Remove FindHIP and use hip-config.cmake instead #526

Remove FindHIP and use hip-config.cmake instead #526

Conversation

davidbeckingsale commented Sep 17, 2021

trws commented Sep 22, 2021

davidbeckingsale commented Sep 22, 2021

kennyweiss left a comment

Choose a reason for hiding this comment

trws commented Sep 24, 2021

davidbeckingsale commented Sep 24, 2021

trws commented Sep 25, 2021 via email

davidbeckingsale commented Oct 29, 2021

kennyweiss left a comment • edited Loading

Choose a reason for hiding this comment

kennyweiss Oct 29, 2021

Choose a reason for hiding this comment

kennyweiss commented Oct 29, 2021 • edited Loading

Config and build lines

Warning (seems safe to ignore)

Linker error

kennyweiss Oct 29, 2021

Choose a reason for hiding this comment

kennyweiss commented Oct 29, 2021

davidbeckingsale commented Nov 1, 2021

DavidPoliakoff commented Nov 9, 2021

rhornung67 commented Nov 9, 2021 • edited Loading

white238 Nov 10, 2021

Choose a reason for hiding this comment

bmhan12 commented Nov 11, 2021

trws commented Nov 16, 2021

davidbeckingsale commented Nov 16, 2021

trws commented Nov 16, 2021

davidbeckingsale commented Nov 16, 2021

cyrush commented Dec 9, 2021

white238 commented Jan 10, 2022 • edited Loading

davidbeckingsale commented Jan 10, 2022

kennyweiss left a comment

Choose a reason for hiding this comment

kennyweiss Jan 10, 2022

Choose a reason for hiding this comment

davidbeckingsale Jan 10, 2022

Choose a reason for hiding this comment

trws Jan 10, 2022

Choose a reason for hiding this comment

davidbeckingsale Jan 10, 2022

Choose a reason for hiding this comment

kennyweiss left a comment •

edited

Loading

kennyweiss commented Oct 29, 2021 •

edited

Loading

rhornung67 commented Nov 9, 2021 •

edited

Loading

white238 commented Jan 10, 2022 •

edited

Loading