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.

Sign up for GitHub

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

Jump to bottom

Installed Examples for Device #919

Merged
merged 7 commits into from
Aug 30, 2022
Merged

Installed Examples for Device #919

merged 7 commits into from
Aug 30, 2022

Conversation

bmhan12
Copy link
Contributor

@bmhan12 bmhan12 commented Aug 29, 2022

This PR:

@bmhan12 bmhan12 added Testing Issues related to testing Axom GPU Issues related to GPU development labels Aug 29, 2022
@bmhan12 bmhan12 self-assigned this Aug 29, 2022
kennyweiss
kennyweiss approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bmhan12 !

src/examples/using-with-cmake/host-config.cmake.in Outdated
Comment on lines 34 to 38
set(CUDA_TOOLKIT_ROOT_DIR "@CUDA_TOOLKIT_ROOT_DIR@" CACHE PATH "")
set(CMAKE_CUDA_COMPILER "@CMAKE_CUDA_COMPILER@" CACHE PATH "")
set(CMAKE_CUDA_HOST_COMPILER "@CMAKE_CUDA_HOST_COMPILER@" CACHE PATH "")
set(CMAKE_CUDA_ARCHITECTURES "@CMAKE_CUDA_ARCHITECTURES@" CACHE STRING "")
set(CMAKE_CUDA_FLAGS "@CMAKE_CUDA_FLAGS@" CACHE STRING "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wrapped in

if(ENABLE_CUDA) 
   ... 
endif()

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an if() check in the CMakeLists.txt to check for device.

I was considering this, but I do not know if it is best practice to put if() checks in our host-configs (technically we do have an if() check that is spack generated in our current host-configs, but the check only matters for when spack is using the host-config).

src/examples/using-with-cmake/host-config.cmake.in Outdated
Comment on lines 42 to 45
set(HIP_ROOT_DIR "@HIP_ROOT_DIR@" CACHE STRING "")
set(HIP_CLANG_PATH "@HIP_CLANG_PATH@" CACHE STRING "")
set(CMAKE_HIP_ARCHITECTURES "@CMAKE_HIP_ARCHITECTURES@" CACHE STRING "")
set(CMAKE_EXE_LINKER_FLAGS "@CMAKE_EXE_LINKER_FLAGS@" CACHE STRING "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, should the HIP-related lines (other than the first) be wrapped in if(ENABLE_HIP) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be #cmakedefines, so they only exist in the case where hip, etc are enabled? They are still a little clunky to use, but seems like it would avoid blank entries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could go either way... I'd be fine with them guarded by an if... or do the fancier solution of printing the whole block in a cmake define like cyrus suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with if guards.

src/examples/using-with-cmake/CMakeLists.txt
Comment on lines +63 to +69
# BEGIN FIXME:
# Create fake empty target, this stops CMake from adding -lcuda_runtime to the link line
add_library(cuda_runtime INTERFACE)

# Create fake empty target, this stops CMake from adding -lblt_hip_runtime to the link line
add_library(blt_hip_runtime INTERFACE)
# END FIXME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@white238 -- is there a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be curious where these are coming from. Axom should be properly exporting it's targets here:

axom/src/cmake/thirdparty/SetupAxomThirdParty.cmake

Lines 282 to 297 in 78d987f

blt_list_append(TO TPL_DEPS ELEMENTS cuda cuda_runtime IF ENABLE_CUDA)
blt_list_append(TO TPL_DEPS ELEMENTS blt_hip blt_hip_runtime IF ENABLE_HIP)
blt_list_append(TO TPL_DEPS ELEMENTS openmp IF ENABLE_OPENMP)
blt_list_append(TO TPL_DEPS ELEMENTS mpi IF ENABLE_MPI)
foreach(dep ${TPL_DEPS})
# If the target is EXPORTABLE, add it to the export set
get_target_property(_is_imported ${dep} IMPORTED)
if(NOT ${_is_imported})
install(TARGETS ${dep}
EXPORT axom-targets
DESTINATION lib)
# Namespace target to avoid conflicts
set_target_properties(${dep} PROPERTIES EXPORT_NAME axom::${dep})
endif()
endforeach()

(side note: this should be replaced with BLT's macro, blt_export_tpl_targets.)

Copy link
Contributor Author

@bmhan12 bmhan12 Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I spent some time with blt_print_target_properties, but it was still unclear to me where this is being pulled in. Maybe once LLNL/blt#590 becomes available it'll be easier to pinpoint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw this is the solution for making this go away unfortunately.

@white238
Copy link
Member

Thanks @bmhan12 !

@bmhan12 bmhan12 merged commit f30d0c8 into develop Aug 30, 2022
@bmhan12 bmhan12 deleted the bugfix/han12/usingEx branch August 30, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyrush cyrush cyrush left review comments

@kennyweiss kennyweiss kennyweiss approved these changes

@white238 white238 white238 approved these changes

@rhornung67 rhornung67 Awaiting requested review from rhornung67

Assignees

@bmhan12 bmhan12

Labels
GPU Issues related to GPU development Testing Issues related to testing Axom
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bmhan12 @white238 @cyrush @kennyweiss