[openmp] Revise IDE folder structure #89750
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
llvmbot
added
openmp:libomp
|
@llvm/pr-subscribers-offload Author: Michael Kruse (Meinersbur) ChangesReviewers of #89153 suggested to break up the patch into per-subproject patches. This is the OpenMP part. See #89153 for the entire series and motivation. Update the folder titles for targets in the monorepository that have not seen taken care of for some time. These are the folders that targets are organized in Visual Studio and XCode (
Full diff: https://github.com/llvm/llvm-project/pull/89750.diff 5 Files Affected:
diff --git a/offload/unittests/CMakeLists.txt b/offload/unittests/CMakeLists.txt
index 73c87b708d25f..c818a3d985ba3 100644
--- a/offload/unittests/CMakeLists.txt
+++ b/offload/unittests/CMakeLists.txt
@@ -1,5 +1,5 @@
add_custom_target(LibomptUnitTests)
-set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Tests/UnitTests")
+set_target_properties(LibomptUnitTests PROPERTIES FOLDER "Tests")
function(add_libompt_unittest test_dirname)
add_unittest(LibomptUnitTests ${test_dirname} ${ARGN})
diff --git a/openmp/CMakeLists.txt b/openmp/CMakeLists.txt
index 95f2425db3ee6..daef2b77fd4dd 100644
--- a/openmp/CMakeLists.txt
+++ b/openmp/CMakeLists.txt
@@ -1,4 +1,5 @@
cmake_minimum_required(VERSION 3.20.0)
+set(LLVM_SUBPROJECT_TITLE "OpenMP")
set(LLVM_COMMON_CMAKE_UTILS ${CMAKE_CURRENT_SOURCE_DIR}/../cmake)
diff --git a/openmp/docs/CMakeLists.txt b/openmp/docs/CMakeLists.txt
index 1e4be31a6f609..4cb9fb486ff34 100644
--- a/openmp/docs/CMakeLists.txt
+++ b/openmp/docs/CMakeLists.txt
@@ -78,6 +78,7 @@ if (LLVM_ENABLE_DOXYGEN)
COMMAND ${DOXYGEN_EXECUTABLE} ${CMAKE_CURRENT_BINARY_DIR}/doxygen.cfg
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
COMMENT "Generating openmp doxygen documentation." VERBATIM)
+ set_target_properties(doxygen-openmp PROPERTIES FOLDER "OpenMP/Docs")
if (LLVM_BUILD_DOCS)
add_dependencies(doxygen doxygen-openmp)
diff --git a/openmp/runtime/cmake/LibompMicroTests.cmake b/openmp/runtime/cmake/LibompMicroTests.cmake
index e8cc218af0c29..6fcde37259931 100644
--- a/openmp/runtime/cmake/LibompMicroTests.cmake
+++ b/openmp/runtime/cmake/LibompMicroTests.cmake
@@ -126,6 +126,7 @@ macro(libomp_test_touch_recipe test_touch_dir)
endmacro()
libomp_append(libomp_test_touch_env "KMP_VERSION=1")
add_custom_target(libomp-test-touch DEPENDS ${libomp_test_touch_targets})
+set_target_properties(libomp-test-touch PROPERTIES FOLDER "OpenMP/Tests")
if(WIN32)
libomp_test_touch_recipe(test-touch-mt)
libomp_test_touch_recipe(test-touch-md)
@@ -135,6 +136,7 @@ endif()
# test-relo
add_custom_target(libomp-test-relo DEPENDS test-relo/.success)
+set_target_properties(libomp-test-relo PROPERTIES FOLDER "OpenMP/Tests")
add_custom_command(
OUTPUT test-relo/.success test-relo/readelf.log
COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/test-relo
@@ -146,6 +148,7 @@ add_custom_command(
# test-execstack
add_custom_target(libomp-test-execstack DEPENDS test-execstack/.success)
+set_target_properties(libomp-test-execstack PROPERTIES FOLDER "OpenMP/Tests")
add_custom_command(
OUTPUT test-execstack/.success
COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/test-execstack
@@ -157,6 +160,7 @@ add_custom_command(
# test-instr
add_custom_target(libomp-test-instr DEPENDS test-instr/.success)
+set_target_properties(libomp-test-instr PROPERTIES FOLDER "OpenMP/Tests")
add_custom_command(
OUTPUT test-instr/.success
COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/test-instr
@@ -168,6 +172,7 @@ add_custom_command(
# test-deps
add_custom_target(libomp-test-deps DEPENDS test-deps/.success)
+set_target_properties(libomp-test-deps PROPERTIES FOLDER "OpenMP/Tests")
set(libomp_expected_library_deps)
if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
set(libomp_expected_library_deps libc.so.7 libthr.so.3 libm.so.5)
diff --git a/openmp/runtime/src/CMakeLists.txt b/openmp/runtime/src/CMakeLists.txt
index a2468d04e60af..963f14bb7ffc2 100644
--- a/openmp/runtime/src/CMakeLists.txt
+++ b/openmp/runtime/src/CMakeLists.txt
@@ -171,6 +171,7 @@ libomp_get_libflags(LIBOMP_CONFIGURED_LIBFLAGS)
# Build libomp library. Add LLVMSupport dependency if building in-tree with libomptarget profiling enabled.
if(OPENMP_STANDALONE_BUILD OR (NOT OPENMP_ENABLE_LIBOMP_PROFILING))
add_library(omp ${LIBOMP_LIBRARY_KIND} ${LIBOMP_SOURCE_FILES})
+ set_property(TARGET omp PROPERTY FOLDER "OpenMP/Libraries")
# Linking command will include libraries in LIBOMP_CONFIGURED_LIBFLAGS
target_link_libraries(omp ${LIBOMP_CONFIGURED_LIBFLAGS} ${LIBOMP_DL_LIBS})
else()
@@ -252,6 +253,7 @@ set(LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER "${LIBOMP_LIBRARY_DIR}" CACHE STRING
# Create *.inc before compiling any sources
# objects depend on : .inc files
add_custom_target(libomp-needed-headers DEPENDS kmp_i18n_id.inc kmp_i18n_default.inc)
+set_target_properties(libomp-needed-headers PROPERTIES FOLDER "OpenMP/Tablegenning")
add_dependencies(omp libomp-needed-headers)
# Windows specific build rules
@@ -293,6 +295,7 @@ if(WIN32)
set(LIBOMP_IMP_LIB_TARGET omp)
set(LIBOMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.def)
add_custom_target(libomp-needed-def-file DEPENDS ${LIBOMP_GENERATED_DEF_FILE})
+ set_target_properties(libomp-needed-def-file PROPERTIES FOLDER "OpenMP/Tablegenning")
add_dependencies(omp libomp-needed-def-file)
# Create the main def file with ordinals to use for building the runtime dll to maintain backwards compatible exports order
@@ -311,6 +314,7 @@ if(WIN32)
# Create the auxiliary def file without ordinals to use for building the import library to import by name
set(LIBOMPIMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.imp.def)
add_custom_target(libompimp-needed-def-file DEPENDS ${LIBOMPIMP_GENERATED_DEF_FILE})
+ set_target_properties(libompimp-needed-def-file PROPERTIES FOLDER "OpenMP/Resources")
add_custom_command(
OUTPUT ${LIBOMPIMP_GENERATED_DEF_FILE}
COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE} -D NOORDINALS
@@ -320,6 +324,7 @@ if(WIN32)
# while this merely generates an import library off a def file, CMAKE still requires it to have a "source" so feed it a dummy one,
# making it a .txt which CMAKE will filter out from the librarian (a .cpp will make lib.exe punt trying to resolve the .def symbols)
add_library(${LIBOMP_IMP_LIB_TARGET} STATIC kmp_dummy.txt)
+ set_target_properties(${LIBOMP_IMP_LIB_TARGET} PROPERTIES FOLDER "OpenMP/Resources")
set_target_properties(${LIBOMP_IMP_LIB_TARGET} PROPERTIES
PREFIX "" SUFFIX "" OUTPUT_NAME "${LIBOMP_IMP_LIB_FILE}" LINKER_LANGUAGE ${LIBOMP_LINKER_LANGUAGE}
STATIC_LIBRARY_OPTIONS "${CMAKE_LINK_DEF_FILE_FLAG}${CMAKE_CURRENT_BINARY_DIR}/${LIBOMPIMP_GENERATED_DEF_FILE}")
@@ -355,6 +360,7 @@ elseif(${LIBOMP_FORTRAN_MODULES})
set(ADDITIONAL_Fortran_FLAGS "-fno-range-check")
endif()
add_custom_target(libomp-mod ALL DEPENDS omp_lib.mod omp_lib_kinds.mod)
+ set_target_properties(libomp-mod PROPERTIES FOLDER "OpenMP/Misc")
libomp_get_fflags(LIBOMP_CONFIGURED_FFLAGS)
if(CMAKE_Fortran_COMPILER_SUPPORTS_F90)
set(LIBOMP_FORTRAN_SOURCE_FILE omp_lib.F90)
@@ -380,6 +386,7 @@ endif()
# Micro test rules for after library has been built (cmake/LibompMicroTests.cmake)
include(LibompMicroTests)
add_custom_target(libomp-micro-tests)
+set_target_properties(libomp-micro-tests PROPERTIES FOLDER "OpenMP/Tests")
if(NOT ${MIC} AND NOT CMAKE_CROSSCOMPILING)
add_dependencies(libomp-micro-tests libomp-test-touch)
endif()
|
Git's rename detection seem to also have moved a part of the patch
shiltian
reviewed
May 21, 2024
| @@ -126,6 +126,7 @@ macro(libomp_test_touch_recipe test_touch_dir) | |||
| endmacro() | |||
| libomp_append(libomp_test_touch_env "KMP_VERSION=1") | |||
| add_custom_target(libomp-test-touch DEPENDS ${libomp_test_touch_targets}) | |||
| set_target_properties(libomp-test-touch PROPERTIES FOLDER "OpenMP/Tests") | |||
There was a problem hiding this comment.
I suppose these are for Windows (and potentially Xcode on macOS), especially those "folders" don't look like anything on Linux. I'd recommend libomp/tests, libomp/docs, etc. We don't call our library "OpenMP" library.
There was a problem hiding this comment.
- The subproject's directory name is
openmp - The README calls it "LLVM OpenMP Libraries".
- The website is openmp.llvm.org, and libomp is not even mentioned. Its title is "LLVM/OpenMP", I think we can drop the "LLVM" prefix within the LLVM repository.
libompd,libomptarget(formerly),omp.h, Archer, and ompopt documentation lives in there too, i.e. this is not the subproject for just libomp.- The prefix for the CMake variable is "OPENMP_"
The folder names require an IDE what display them, none of them are available for Linux. They are also not directories in the filesystem, case should not matter.
I could call it "OpenMP Libraries" if that makes it clearer.
There was a problem hiding this comment.
My point is, when it comes to directory (except the top level folder), we don't call it "OpenMP". I don't have a strong objection, but just find libomp more conventional. I'll leave it up to you.
shiltian
approved these changes
May 22, 2024
| @@ -126,6 +126,7 @@ macro(libomp_test_touch_recipe test_touch_dir) | |||
| endmacro() | |||
| libomp_append(libomp_test_touch_env "KMP_VERSION=1") | |||
| add_custom_target(libomp-test-touch DEPENDS ${libomp_test_touch_targets}) | |||
| set_target_properties(libomp-test-touch PROPERTIES FOLDER "OpenMP/Tests") | |||
There was a problem hiding this comment.
My point is, when it comes to directory (except the top level folder), we don't call it "OpenMP". I don't have a strong objection, but just find libomp more conventional. I'll leave it up to you.
openmp/runtime/src/CMakeLists.txt
Outdated
| @@ -292,6 +294,7 @@ if(WIN32) | |||
| set(LIBOMP_IMP_LIB_TARGET omp) | |||
| set(LIBOMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.def) | |||
| add_custom_target(libomp-needed-def-file DEPENDS ${LIBOMP_GENERATED_DEF_FILE}) | |||
| set_target_properties(libomp-needed-def-file PROPERTIES FOLDER "OpenMP/Codegenning") | |||
There was a problem hiding this comment.
Codegenning is really weird. I don't see this word anywhere else in LLVM project. Probably just OpenMP/CodeGen.
There was a problem hiding this comment.
For LLVM/Clang/MLIR the folder is already named "Tablegenning" since forever. Since tablegen is not involved here, I found "Codegenning" more appropriate.
Meinersbur
changed the base branch from
users/meinersbur/ide_folders_llvm
to
main
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewers of #89153 suggested to break up the patch into per-subproject patches. This is the OpenMP part. See #89153 for the entire series and motivation.
Update the folder titles for targets in the monorepository that have not seen taken care of for some time. These are the folders that targets are organized in Visual Studio and XCode (
set_property(TARGET <target> PROPERTY FOLDER "<title>")) when using the respective CMake's IDE generator.set_property/set_target_property, but are still necessary whenadd_custom_target,add_executable,add_library, etc. are used. A LLVM_SUBPROJECT_TITLE definition is used for that in each subproject's root CMakeLists.txt.