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.

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

Collected small changes and bugfixes #2023

Merged
merged 32 commits into from Apr 28, 2020

Conversation

akohlmey
Copy link
Member

@akohlmey akohlmey commented Apr 25, 2020

Summary

This PR combines multiple small changes that do not warrant a pull request of their own.

Author(s)

Axel Kohlmeyer (Temple U)

Related Issues

Includes and thus closes #2018
Includes and thus closes #2024
Closes #2028
Closes #2031
Includes and thus closes #2034

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

No known issues.

Implementation Notes

This PR addresses the following issues:

  • make a check in pair style drip compatible with suffix styles
  • correct logic in pair style hybrid to identify non-matching coulomb styles
  • rename dummy file ... in src/MAKE/MINE to .gitignore to avoid causing trouble for people using windows
  • update MPI detection. Rather than singling out the Windows system, we treat cross-compilation as a special case and then include the MPI4WIN.cmake file only when also building for Windows.
    This should enable building on Windows natively with MinGW and a local MPI installation. See issue [BUG] Build with Win4MPI fails on Windows #2028
  • include a new combined singularity definition file for building with GPU support
  • correct multiple issues when building with CMake with the GPU package (related to HIP, also when compiling without MPI)
  • new CMake preset for enabling hipcc to compile for AMD GPUs
  • update legacy makefile for HIP
  • correct issues with CMake and LATTE

Post Submission Checklist

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system

cmake/CMakeLists.txt Outdated Show resolved Hide resolved
@akohlmey akohlmey self-assigned this Apr 26, 2020
@akohlmey akohlmey added tools and removed test_runs Enable to trigger run tests labels Apr 27, 2020
@akohlmey akohlmey marked this pull request as ready for review April 27, 2020 20:13
@akohlmey akohlmey requested a review from rbberger as a code owner April 27, 2020 20:13
@akohlmey akohlmey requested a review from sjplimp April 27, 2020 20:13
@junghans junghans self-requested a review April 28, 2020 00:32
Copy link
Member Author

@akohlmey akohlmey left a comment

Choose a reason for hiding this comment

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

@junghans is there a better way of doing this?
When compiling the GPU package with -DBUILD_MPI=off and many other packages added somehow CMake picks up the path to a regular MPI package and thus linkage to the GPU library fails due to differences in the data type for the MPI communicator (with OpenMPI). With the changes in this commit things work again but it feels wrong. Shouldn't using the STUBS library replace any MPI library? Like it is done with the downloaded files for Windows?

(ASPHERE BODY CLASS2 COLLOID COMPRESS CORESHELL DIPOLE GPU GRANULAR KSPACE
MANYBODY MC MISC MOLECULE OPT PERI POEMS PYTHON QEQ REPLICA RIGID SHOCK SNAP
SPIN SRD USER-AWPMD USER-BOCS USER-CGDNA USER-CGSDK USER-COLVARS USER-DIFFRACTION USER-DPD USER-DRUDE USER-EFF USER-FEP USER-H5MD USER-INTEL USER-MANIFOLD USER-MEAMC USER-MESODPD USER-MISC USER-MOFFF USER-MOLFILE USER-OMP USER-PHONON USER-PTM USER-QTB USER-REACTION USER-REAXC USER-SDPD
USER-SMTBQ USER-SPH USER-TALLY USER-UEF USER-YAFF VORONOI)

Copy link
Member Author

@akohlmey akohlmey left a comment

Choose a reason for hiding this comment

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

@junghans I've seen this failing for flags like -g or -march=native and some others. For the latter I can see that this gives problems because of the = sign. We should either find a better way to do this or just drop it and leave it to the user and just make certain that the flags that are enabled by default, if the CMAKE_TUNE_FLAGS variable is not set, work with all supported compilers that match the compiler-ids we are comparing to.

@@ -99,6 +99,9 @@ if(GPU_API STREQUAL "CUDA")
add_library(gpu STATIC ${GPU_LIB_SOURCES} ${GPU_LIB_CUDPP_SOURCES} ${GPU_OBJS})
target_link_libraries(gpu PRIVATE ${CUDA_LIBRARIES} ${CUDA_CUDA_LIBRARY})
target_include_directories(gpu PRIVATE ${LAMMPS_LIB_BINARY_DIR}/gpu ${CUDA_INCLUDE_DIRS})
if(NOT BUILD_MPI)
target_include_directories(gpu PRIVATE ${LAMMPS_SOURCE_DIR}/STUBS)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Could you just do target_link_libraries(gpu PRIVATE MPI::MPI_CXX) at the very bottom on GPU.cmake?

Copy link
Member Author

Choose a reason for hiding this comment

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

that is already there

Copy link
Member

Choose a reason for hiding this comment

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

I think here is what is going on and I had similar discussion with @rbberger on #1956 as well. find_package(MPI) will create an incomplete MPI::MPI_CXX target and hence the aliasing of mpistubs to MPI::MPI_CXX will not forward the include directories.

One workaround to this would be to not use a target in the MPI:: namespace, e.g. LAMMPS::MPI, however aliasing imported target, like MPI::MPI_CXX (from find_package(MPI) ) isn't supported in CMake-3.10 yet, so this wasn't an option either.

See akohlmey#65 for slightly cleaner version of what you had.

cmake/CMakeLists.txt Outdated Show resolved Hide resolved
@akohlmey akohlmey merged commit 2e07345 into lammps:master Apr 28, 2020
@akohlmey akohlmey deleted the collected-small-changes branch April 28, 2020 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants