add option to use mpi imported targets #482

cyrush · 2021-04-14T18:45:00Z

Adds an option to use the modern imported targets provided by FindMPI.

This solves an issue with BLT related to transitive mpi deps and -pthreads vs cuda.

The issue:

You build a TPL that doesn't have cuda support. In that case, BLT does not (cannot) apply any of the cuda pthreads protection logic. (As far as I know we can't use generator exprs that involve non-enabled languages)
You now use this TPL in a lib or application that does need CUDA support. The flags that are tied to MPI are already baked in to the first lib, so nvcc sees pthreads and gives up.

Using the targets imported by FindMPI solves this issue, b/c those targets expand to include the proper guards when FindMPI is called.

I don't know the exact point at where it changed, but pthreads is added by FindMPI in newer versions of CMake. It does not happen in CMake 3.14. We may eventually want to make this the default when newer CMake is used.

TODO:

Better name for the option
Add some docs to https://llnl-blt.readthedocs.io/en/develop/tutorial/common_hpc_dependencies.html#commonhpcdependencies

white238 · 2021-04-14T18:47:39Z

cmake/thirdparty/SetupMPI.cmake

-                   LINK_FLAGS    ${_mpi_link_flags} 
-                   EXPORTABLE    ${BLT_EXPORT_THIRDPARTY})
+
+if(BLT_USE_MPI_IMPORTED_TARGETS)


imported is a bit overloaded in this context.

maybe "BLT_USE_CMAKE_MPI_TARGETS"?

the other MPI options are all have the BLT_MPI prefix, like BLT_MPI_COMPILE_FLAGS

how about:
BLT_MPI_USE_FINDMPI_CMAKE_TARGETS

That works.. after this is proven to solve you problem, we should handle the overriding case where the user provides includes/link flags/etc.

Either by erroring out or overriding the target's properties

or
BLT_MPI_USE_FIND_MPI_CMAKE_TARGETS

BLT_USE_FIND_MPI_TARGETS?

cyrush · 2021-04-15T23:43:04Z

@white238 pushed docs, error checks and improvements. I also verified this does resolve the issue.
Let me know if I should make more tweaks.

white238 · 2021-04-16T01:48:06Z

cmake/thirdparty/SetupMPI.cmake

+#
+# TODO: should this be BLT_ENABLE_FIND_MPI?
+#
+if(ENABLE_FIND_MPI)


Can you remove line 34 if you want to add this?

Yea this was a very early option. It should be prefixed but I've been hesitant to change it due to legacy. But honestly no one uses that I know anymore. The main use case was BGQ.

removed line 34 and the TODO

white238 · 2021-04-16T01:52:45Z

cmake/thirdparty/SetupMPI.cmake

+                       COMPILE_FLAGS ${_mpi_compile_flags}
+                       LINK_FLAGS    ${_mpi_link_flags} 
+                       EXPORTABLE    ${BLT_EXPORT_THIRDPARTY})
+endif()


Add a new line here please.

white238 · 2021-04-16T01:53:20Z

docs/tutorial/common_hpc_dependencies.rst

@@ -86,6 +86,11 @@ Test. ``test_2.cpp`` provides an example driver for MPI with GoogleTest.
  BLT also has the variable ``ENABLE_FIND_MPI`` which turns off all CMake's ``FindMPI``
  logic and then uses the MPI wrapper directly when you provide them as the default
  compilers.
+
+  BLT also provides ``BLT_USE_FIND_MPI_TARGETS`` that will directly use the
+  ``MPI::MPI_C`` and ``MPI::MPI_CXX`` targets provided by CMake's ``FindMPI``.


Please add that it those targets are able to be referenced under the unified name of mpi

two backticks

two backticks times two.

kennyweiss

Looks good @cyrush

Please also update the RELEASE_NOTES

kennyweiss · 2021-04-16T01:49:54Z

cmake/thirdparty/SetupMPI.cmake

@@ -33,6 +33,29 @@ set(_mpi_link_flags )

 message(STATUS "Enable FindMPI:  ${ENABLE_FIND_MPI}")

+#
+# TODO: should this be BLT_ENABLE_FIND_MPI?


TODO: should this be BLT_ENABLE_FIND_MPI?

I agree that we should convert it to BLT_ENABLE_FIND_MPI and deprecate ENABLE_FIND_MPI, e.g. via a loud warning at first.

Perhaps we should do the same for many/most blt ENABLE_ variables at the same time?

It would be a good time to do it since we just did the 0.4.0 release.

Added: #483 to track this name change, won't do in this PR

kennyweiss · 2021-04-16T01:51:23Z

cmake/thirdparty/SetupMPI.cmake

+
+
+
+if(NOT ENABLE_FIND_MPI AND BLT_USE_FIND_MPI_TARGETS)


Seems like we could handle this better by converting BLT_USE_FIND_MPI_TARGETS to a cmake_dependent_option

I tried cmake_dependent_option. It will just flip BLT_USE_FIND_MPI_TARGETS to OFF, when ENABLE_FIND_MPI is OFF. I think an explicit error is a better approach here -- better to error when things aren't consistent than to drive on to surprise town.

kennyweiss · 2021-04-16T01:52:30Z

cmake/thirdparty/SetupMPI.cmake

 if (BLT_MPI_COMPILE_FLAGS)
+    if(BLT_USE_FIND_MPI_TARGETS)


Can you please convert this to a foreach() ?
That way it is easier to see that it is the same logic.

Each error case here provides a detailed unique error message. It's not just the var names that are different, so var indirection in a for loop won't get you the same result.

Looking at the full section in the file -- the logic seems easy to parse. I feel a cmake for loop would not improve the readability. If you have an idea of how it could work, lets explore in a future PR.

This reverts commit 68c98b7.

add option to use mpi imported targets

46ca290

white238 reviewed Apr 14, 2021

View reviewed changes

cyrush added 2 commits April 15, 2021 11:11

change option name and add error checking

bc31ba6

blurb

72a26ee

cyrush requested review from kennyweiss and joshessman-llnl April 15, 2021 23:43

white238 reviewed Apr 16, 2021

View reviewed changes

kennyweiss approved these changes Apr 16, 2021

View reviewed changes

cyrush added 4 commits April 16, 2021 11:41

pr feedback

7014106

space

3746564

now with more backticks

efd6b3f

more backticks for the backticks auditors

9b53e38

cyrush merged commit 68c98b7 into develop Apr 16, 2021

cyrush deleted the task/2021_04_find_mpi_fixes branch April 16, 2021 20:06

cyrush added a commit that referenced this pull request Apr 22, 2021

Revert "add option to use mpi imported targets (#482)"

0fee231

This reverts commit 68c98b7.

cyrush mentioned this pull request Apr 22, 2021

Revert "add option to use mpi imported targets" #486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to use mpi imported targets #482

add option to use mpi imported targets #482

cyrush commented Apr 14, 2021 •

edited

Loading

white238 Apr 14, 2021

white238 Apr 14, 2021

cyrush Apr 15, 2021

white238 Apr 15, 2021 •

edited

Loading

cyrush Apr 15, 2021

white238 Apr 15, 2021

cyrush commented Apr 15, 2021

white238 Apr 16, 2021

cyrush Apr 16, 2021

white238 Apr 16, 2021

cyrush Apr 16, 2021

white238 Apr 16, 2021

cyrush Apr 16, 2021

white238 Apr 16, 2021

cyrush Apr 16, 2021

kennyweiss left a comment

kennyweiss Apr 16, 2021

white238 Apr 16, 2021

cyrush Apr 16, 2021 •

edited

Loading

kennyweiss Apr 16, 2021

cyrush Apr 16, 2021

kennyweiss Apr 16, 2021

cyrush Apr 16, 2021

cyrush Apr 16, 2021

add option to use mpi imported targets #482

add option to use mpi imported targets #482

Conversation

cyrush commented Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

white238 Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush commented Apr 15, 2021

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

kennyweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush Apr 16, 2021 • edited Loading

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

cyrush commented Apr 14, 2021 •

edited

Loading

white238 Apr 15, 2021 •

edited

Loading

cyrush Apr 16, 2021 •

edited

Loading