This is an archive of the discontinued LLVM Phabricator instance.

Differential D84148

[openmp] Fix libomp.so copy in multi-config generators.
ClosedPublic

Authored by DavidTruby on Jul 20 2020, 4:18 AM.

Details

Reviewers
jdoerfert
AndreyChurbanov
protze.joachim
Commits
rGbb099c87abbf: [openmp] Don't copy exports into the source folder by default.
Summary

This changes the copy command for libomp.so to use the output of the target as
the source of the copy, rather than trying to find it based on
${LIBOMP_LIBRARY_DIR}, which appears to be incorrect in multi-config generator
builds.

Diff Detail

Unit TestsFailed

TimeTest
3,400 mslinux > libomp.env::Unknown Unit Message ("")
1,190 mslinux > libomp.worksharing/for::Unknown Unit Message ("")

Event Timeline

DavidTruby created this revision.Jul 20 2020, 4:18 AM
Herald added a reviewer: jdoerfert. · View Herald TranscriptJul 20 2020, 4:18 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: openmp-commits, sstefan1, guansong and 2 others. · View Herald Transcript
DavidTruby added a comment.Jul 20 2020, 4:20 AM

I'm a little uncomfortable with this line copying things from the build directory to the source directory; I believe the source directory should be able to be immutable and builds should still work. Part of the principle of having out-of-tree builds is that the source tree should remain unmodified.
Regardless, I haven't changed the behaviour here as I don't know why the library gets copied to the source directory. Perhaps someone could clarify that? Until then I am just replicating the existing behaviour but with it also working for multi-config generators.

Harbormaster failed remote builds in B64888: Diff 279179!Jul 20 2020, 4:54 AM
protze.joachim added a subscriber: protze.joachim.Jul 20 2020, 5:15 AM

What would be needed to remove this strange copying step? Do the tests really rely on these directories?
I could run the tests successfully with cmake -DLIBOMP_COPY_EXPORTS=FALSE.

protze.joachim added subscribers: AndreyChurbanov, jlpeyton.Jul 20 2020, 5:24 AM

I think this export directory structure is a remainder from the original configure based Intel/OpenMP runtime.
@jlpeyton @AndreyChurbanov is this step still needed?

If Intel relies on this export directory, can we disable by default and you enable it for internal purpose?

AndreyChurbanov added a comment.Jul 20 2020, 12:57 PM
In D84148#2161732, @protze.joachim wrote:

I think this export directory structure is a remainder from the original configure based Intel/OpenMP runtime.
@jlpeyton @AndreyChurbanov is this step still needed?

If Intel relies on this export directory, can we disable by default and you enable it for internal purpose?

I think this is historical artifact that can be removed nowadays.
Maybe, to be on the safe side, we can set LIBOMP_COPY_EXPORTS to false by default, as you suggested.
Then later we can remove the copying functionality at all.

protze.joachim added a comment.Jul 21 2020, 6:18 AM

@DavidTruby please change the default to false and I'll accept the patch.

DavidTruby updated this revision to Diff 279506.Jul 21 2020, 6:44 AM

Disable libomp export copies by default.

Harbormaster failed remote builds in B65075: Diff 279506!Jul 21 2020, 7:09 AM
AndreyChurbanov accepted this revision.Jul 21 2020, 7:14 AM

LGTM

This revision is now accepted and ready to land.Jul 21 2020, 7:14 AM
saiislam mentioned this in D84241: [OpenMP] Temporarily disable failing runtime tests for clang-12.Jul 21 2020, 8:25 AM
protze.joachim accepted this revision.EditedJul 21 2020, 2:51 PM

Thanks for the update, lgtm

Do you have commit rights to land the patch?

Closed by commit rGbb099c87abbf: [openmp] Don't copy exports into the source folder by default. (authored by DavidTruby). · Explain WhyJul 24 2020, 6:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
openmp/
runtime/
2 lines
cmake/
2 lines

Diff 279506

openmp/runtime/CMakeLists.txt

Show First 20 Lines • Show All 108 Lines • ▼ Show 20 Linesset(LIBOMP_LIBFLAGS "" CACHE STRING
"Appended user specified linked libs flags. (e.g., -lm)") "Appended user specified linked libs flags. (e.g., -lm)")
set(LIBOMP_FFLAGS "" CACHE STRING set(LIBOMP_FFLAGS "" CACHE STRING
"Appended user specified Fortran compiler flags. These are only used if LIBOMP_FORTRAN_MODULES==TRUE.") "Appended user specified Fortran compiler flags. These are only used if LIBOMP_FORTRAN_MODULES==TRUE.")
# Should the libomp library and generated headers be copied into the original source exports/ directory # Should the libomp library and generated headers be copied into the original source exports/ directory
# Turning this to FALSE aids parallel builds to not interfere with each other. # Turning this to FALSE aids parallel builds to not interfere with each other.
# Currently, the testsuite module expects the just built OpenMP library to be located inside the exports/ # Currently, the testsuite module expects the just built OpenMP library to be located inside the exports/
# directory. TODO: have testsuite run under llvm-lit directly. We can then get rid of copying to exports/ # directory. TODO: have testsuite run under llvm-lit directly. We can then get rid of copying to exports/
set(LIBOMP_COPY_EXPORTS TRUE CACHE STRING set(LIBOMP_COPY_EXPORTS FALSE CACHE STRING
"Should exports be copied into source exports/ directory?") "Should exports be copied into source exports/ directory?")
# HWLOC-support # HWLOC-support
set(LIBOMP_USE_HWLOC FALSE CACHE BOOL set(LIBOMP_USE_HWLOC FALSE CACHE BOOL
"Use Hwloc (http://www.open-mpi.org/projects/hwloc/) library for affinity?") "Use Hwloc (http://www.open-mpi.org/projects/hwloc/) library for affinity?")
set(LIBOMP_HWLOC_INSTALL_DIR /usr/local CACHE PATH set(LIBOMP_HWLOC_INSTALL_DIR /usr/local CACHE PATH
"Install path for hwloc library") "Install path for hwloc library")
▲ Show 20 Lines • Show All 261 Lines • Show Last 20 Lines

openmp/runtime/cmake/LibompExports.cmake

Show First 20 Lines • Show All 72 Lines • ▼ Show 20 Lines
else() else()
get_target_property(LIBOMP_OUTPUT_DIRECTORY omp LIBRARY_OUTPUT_DIRECTORY) get_target_property(LIBOMP_OUTPUT_DIRECTORY omp LIBRARY_OUTPUT_DIRECTORY)
endif() endif()
if(NOT LIBOMP_OUTPUT_DIRECTORY) if(NOT LIBOMP_OUTPUT_DIRECTORY)
set(LIBOMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) set(LIBOMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
endif() endif()
add_custom_command(TARGET omp POST_BUILD add_custom_command(TARGET omp POST_BUILD
COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBOMP_EXPORTS_LIB_DIR} COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBOMP_EXPORTS_LIB_DIR}
COMMAND ${CMAKE_COMMAND} -E copy ${LIBOMP_OUTPUT_DIRECTORY}/${LIBOMP_LIB_FILE} ${LIBOMP_EXPORTS_LIB_DIR} COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:omp> ${LIBOMP_EXPORTS_LIB_DIR}
) )
# Copy Windows import library into exports/ directory post build # Copy Windows import library into exports/ directory post build
if(WIN32) if(WIN32)
get_target_property(LIBOMPIMP_OUTPUT_DIRECTORY ompimp ARCHIVE_OUTPUT_DIRECTORY) get_target_property(LIBOMPIMP_OUTPUT_DIRECTORY ompimp ARCHIVE_OUTPUT_DIRECTORY)
if(NOT LIBOMPIMP_OUTPUT_DIRECTORY) if(NOT LIBOMPIMP_OUTPUT_DIRECTORY)
set(LIBOMPIMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) set(LIBOMPIMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
endif() endif()
add_custom_command(TARGET ompimp POST_BUILD add_custom_command(TARGET ompimp POST_BUILD
COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBOMP_EXPORTS_LIB_DIR} COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBOMP_EXPORTS_LIB_DIR}
COMMAND ${CMAKE_COMMAND} -E copy ${LIBOMPIMP_OUTPUT_DIRECTORY}/${LIBOMP_IMP_LIB_FILE} ${LIBOMP_EXPORTS_LIB_DIR} COMMAND ${CMAKE_COMMAND} -E copy ${LIBOMPIMP_OUTPUT_DIRECTORY}/${LIBOMP_IMP_LIB_FILE} ${LIBOMP_EXPORTS_LIB_DIR}
) )
endif() endif()