[Openmp-commits] [PATCH] LLVM OpenMP CMake Overhaul
Chandler Carruth
chandlerc at gmail.com
Thu Jun 25 00:22:10 PDT 2015
Full pass through this. Thanks again for working on it, this is definitely starting to really improve things.
REPOSITORY
rL LLVM
================
Comment at: runtime/CMakeLists.txt:30-44
@@ -62,18 +29,17 @@
# Below, cmake will try and determine the operating system and architecture for you.
-# These values are set in CMakeCache.txt when cmake is first run (-Dvar_name=... will take precedence)
-# parameter | default value
-# ----------------------------
-# Right now, this build system considers os=lin to mean "Unix-like that is not MAC"
+# Right now, this build system considers LIBOMP_OS=lin to mean "Unix-like that is not MAC"
# Apple goes first because CMake considers Mac to be a Unix based
# operating system, while libomp considers it a special case
if(${APPLE})
- set(temp_os mac)
-elseif(${UNIX})
- set(temp_os lin)
+ set(LIBOMP_DETECTED_OS mac)
elseif(${WIN32})
- set(temp_os win)
+ set(LIBOMP_DETECTED_OS win)
+elseif(${UNIX})
+ set(LIBOMP_DETECTED_OS lin)
else()
- set(temp_os lin)
+ set(LIBOMP_DETECTED_OS lin)
endif()
+set(LIBOMP_OS ${LIBOMP_DETECTED_OS} CACHE STRING
+ "The operating system to build for (lin/mac/win)")
----------------
CMake already provides variables for querying the OS - why define your own?
================
Comment at: runtime/CMakeLists.txt:95-96
@@ -94,4 +94,4 @@
"Intel(R) Many Integrated Core Architecture (Intel(R) MIC Architecture) (knf/knc). Ignored if not Intel(R) MIC Architecture build.")
-set(LIBOMP_FORTRAN_MODULES false CACHE BOOL
+set(LIBOMP_FORTRAN_MODULES FALSE CACHE BOOL
"Create Fortran module files? (requires fortran compiler)")
----------------
Oh my, I didn't realize that the runtime actually compiled Fortran code.
I think this is pretty bad. It means the particular API exported by the runtime depends on what set of frontends are available. Which in turn means we need folks to acquire a fortran frontend when prepping a build of the runtime to give to others.
I'm not actually a fortran expert, so this may be a naive question, but I thought there was better compatibility between fortran and C and it was possible to define a fortran interface from C... That would certainly be helpful here.
Assuming that's not how it works, and we need to build a fortran component, would it make sense to have it be a separate library name so that it is clear that the C openmp runtime only requires a C compiler to build, and the Fortran bits live in a runtime stacked on top of it and require a Fortran frontend to build?
Probably not for this patch, but something I'd really like to understand.
================
Comment at: runtime/CMakeLists.txt:98-110
@@ -97,18 +97,15 @@
-# - These tests are little tests performed after the library is formed.
-# - The library won't be copied to the exports directory
-# until it has passed/skipped all below tests
-# - To skip these tests, just pass -DLIBOMP_MICRO_TESTS=OFF to cmake
-set(LIBOMP_TEST_TOUCH true CACHE BOOL
+# - These tests are small optional tests performed after the library is formed.
+set(LIBOMP_TEST_TOUCH TRUE CACHE BOOL
"Perform a small touch test?")
-set(LIBOMP_TEST_RELO true CACHE BOOL
+set(LIBOMP_TEST_RELO TRUE CACHE BOOL
"Perform a relocation test for dynamic libraries?")
-set(LIBOMP_TEST_EXECSTACK true CACHE BOOL
+set(LIBOMP_TEST_EXECSTACK TRUE CACHE BOOL
"Perform a execstack test for linux dynamic libraries?")
-set(LIBOMP_TEST_INSTR true CACHE BOOL
+set(LIBOMP_TEST_INSTR TRUE CACHE BOOL
"Perform an instruction test for Intel(R) MIC Architecture libraries?")
-set(LIBOMP_TEST_DEPS true CACHE BOOL
+set(LIBOMP_TEST_DEPS TRUE CACHE BOOL
"Perform a library dependency test?")
-set(LIBOMP_MICRO_TESTS false CACHE BOOL
+set(LIBOMP_MICRO_TESTS TRUE CACHE BOOL
"Perform touch, relo, execstack, instr, and deps tests?")
----------------
Why provide cache settings for any of these? It seems like unnecessary complexity.
================
Comment at: runtime/CMakeLists.txt:136-137
@@ -152,4 +135,4 @@
# 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.
-set(LIBOMP_COPY_EXPORTS true CACHE STRING
+# Turning this to FALSE aids parallel builds to not interfere with each other.
+set(LIBOMP_COPY_EXPORTS TRUE CACHE STRING
"Should exports be copied into source exports/ directory?")
----------------
Should this really be true? Is this important to support? There isn't much context here about why this is needed.
================
Comment at: runtime/CMakeLists.txt:140-142
@@ -156,39 +139,5 @@
-# - Allow three build types: Release, Debug, RelWithDebInfo (these relate to build.pl's release, debug, and diag settings respectively)
-# - default is Release (when CMAKE_BUILD_TYPE is not defined)
-# - CMAKE_BUILD_TYPE affects the -O and -g flags (CMake magically includes correct version of them on per compiler basis)
-# - typical: Release = -O3 -DNDEBUG
-# RelWithDebInfo = -O2 -g -DNDEBUG
-# Debug = -g
-if(CMAKE_BUILD_TYPE)
- # CMAKE_BUILD_TYPE was defined, check for validity
- string(TOLOWER "${CMAKE_BUILD_TYPE}" cmake_build_type_lowercase)
- check_variable(cmake_build_type_lowercase "${build_type_possible_values}")
-else()
- # CMAKE_BUILD_TYPE was not defined, set default to Release
- unset(CMAKE_BUILD_TYPE CACHE)
- set(CMAKE_BUILD_TYPE Release CACHE STRING
- "Choose the type of build, options are: Release/Debug/RelWithDebInfo")
- string(TOLOWER "${CMAKE_BUILD_TYPE}" cmake_build_type_lowercase)
- check_variable(cmake_build_type_lowercase "${build_type_possible_values}")
-endif()
-
-if(${LIBOMP_STANDALONE_BUILD})
- # Allow user to choose a suffix for the installation directory, or if part of
- # LLVM build then just use LLVM_LIBDIR_SUFFIX
- set(LIBOMP_LIBDIR_SUFFIX "" CACHE STRING
- "suffix of lib installation directory e.g., 64 => lib64")
- # Should assertions be enabled? They are on by default, or it part of
- # LLVM build then just use LLVM_ENABLE_ASSERTIONS
- set(LIBOMP_ENABLE_ASSERTIONS TRUE CACHE BOOL
- "enable assertions?")
-else()
- set(LIBOMP_LIBDIR_SUFFIX ${LLVM_LIBDIR_SUFFIX})
- set(LIBOMP_ENABLE_ASSERTIONS ${LLVM_ENABLE_ASSERTIONS})
-endif()
-
-# Check valid values
-check_variable(LIBOMP_OS "${os_possible_values}")
-check_variable(LIBOMP_ARCH "${arch_possible_values}")
-check_variable(LIBOMP_OMP_VERSION "${omp_version_possible_values}")
-check_variable(LIBOMP_LIB_TYPE "${lib_type_possible_values}")
+# CMAKE_BUILD_TYPE was not defined, set default to Release
+if(NOT CMAKE_BUILD_TYPE)
+ set(CMAKE_BUILD_TYPE Release)
+endif()
----------------
None of the other subprojects do this, only the top-level LLVM project and it sets it to Debug. It seems a bit odd to start doing this and in the other direction. Any particular motivation?
================
Comment at: runtime/CMakeLists.txt:144
@@ +143,3 @@
+endif()
+string(TOLOWER "${CMAKE_BUILD_TYPE}" libomp_build_type_lowercase)
+
----------------
I would suggest creating this below, where you actually need and use it.
================
Comment at: runtime/CMakeLists.txt:146-158
@@ -195,4 +145,15 @@
+
+# Check valid values of configuration parameters
+set(libomp_os_possible_values lin mac win)
+set(libomp_arch_possible_values 32e x86_64 32 i386 arm ppc64 ppc64le aarch64 mic)
+set(libomp_omp_version_possible_values 41 40 30)
+set(libomp_lib_type_possible_values normal profile stubs)
+set(libomp_mic_arch_possible_values knf knc)
+libomp_check_variable(LIBOMP_OS "${libomp_os_possible_values}")
+libomp_check_variable(LIBOMP_ARCH "${libomp_arch_possible_values}")
+libomp_check_variable(LIBOMP_OMP_VERSION "${libomp_omp_version_possible_values}")
+libomp_check_variable(LIBOMP_LIB_TYPE "${libomp_lib_type_possible_values}")
if("${LIBOMP_ARCH}" STREQUAL "mic")
- check_variable(LIBOMP_MIC_ARCH "${mic_arch_possible_values}")
+ libomp_check_variable(LIBOMP_MIC_ARCH "${libomp_mic_arch_possible_values}")
endif()
# Get the build number from kmp_version.c
----------------
If you want to do this, it would seem better to do it adjacent to the code that sets up the variables. I would also skip the separate variable for the possible values.
================
Comment at: runtime/CMakeLists.txt:165-170
@@ -204,10 +164,8 @@
-#################################################################
# Set some useful flags variables for other parts of cmake to use
# Operating System
set(LINUX FALSE)
set(MAC FALSE)
set(WINDOWS FALSE)
set(MIC FALSE)
if("${LIBOMP_OS}" STREQUAL "lin")
----------------
At this point we have three different ways to detect Linux vs. Mac. =/ This doesn't seem good at all.
================
Comment at: runtime/CMakeLists.txt:235-239
@@ -280,29 +234,7 @@
-###############################################
-# Features for compilation and build in general
-
-# - Does the compiler support a 128-bit floating point data type? Default is false
-# - If a compiler does, then change it in the CMakeCache.txt file (or using the cmake GUI)
-# or send to cmake -DCOMPILER_SUPPORTS_QUAD_PRECISION=true
-# - If COMPILER_SUPPORTS_QUAD_PRECISION is true, then a corresponding COMPILER_QUAD_TYPE must be given
-# This is the compiler's quad-precision data type.
-# ** TODO: This isn't complete yet. Finish it. Requires changing macros in kmp_os.h **
-set(LIBOMP_COMPILER_SUPPORTS_QUAD_PRECISION false CACHE BOOL
- "*INCOMPLETE* Does the compiler support a 128-bit floating point type?")
-set(LIBOMP_COMPILER_QUAD_TYPE "" CACHE STRING
- "*INCOMPLETE* The quad precision data type (e.g., for gcc, __float128)")
-
-# - Should the orignal build rules for builds be used? (cmake/OriginalBuildRules.cmake). This setting is off by default.
-# - This always compiles with -g. And if it is a release build, the debug info is stripped out via objcopy and put into libomp.dbg.
-set(LIBOMP_USE_BUILDPL_RULES false CACHE BOOL
- "Should the build follow build.pl rules/recipes?")
-
-# - Should the build use the predefined linker flags (OS-dependent) in CommonFlags.cmake?
-# - these predefined linker flags should work for Windows, Mac, and True Linux for the most popular compilers/linkers
-set(LIBOMP_USE_PREDEFINED_LINKER_FLAGS true CACHE BOOL
- "Should the build use the predefined linker flags in CommonFlags.cmake?")
-
-# - On multinode systems, larger alignment is desired to avoid false sharing
-set(LIBOMP_USE_INTERNODE_ALIGNMENT false CACHE BOOL
- "Should larger alignment (4096 bytes) be used for some locks and data structures?")
+# Setting directory names
+set(LIBOMP_BASE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set(LIBOMP_SRC_DIR ${LIBOMP_BASE_DIR}/src)
+set(LIBOMP_TOOLS_DIR ${LIBOMP_BASE_DIR}/tools)
+set(LIBOMP_INC_DIR ${LIBOMP_SRC_DIR}/include/${LIBOMP_OMP_VERSION})
----------------
I'm curious why these variables are needed?
================
Comment at: runtime/CMakeLists.txt:266-268
@@ -404,6 +265,5 @@
-############################
-# Setting final library name
-set(lib_item "libomp")
-if(${PROFILE_LIBRARY})
- set(lib_item "${lib_item}prof")
+# On multinode systems, larger alignment is desired to avoid false sharing
+set(LIBOMP_USE_INTERNODE_ALIGNMENT FALSE CACHE BOOL
+ "Should larger alignment (4096 bytes) be used for some locks and data structures?")
+
----------------
This seems really strange to be a CMake thing and not something in the source code.
================
Comment at: runtime/CMakeLists.txt:294
@@ +293,3 @@
+
+# Error checking the configuration
+if(LIBOMP_USE_QUAD_PRECISION AND (NOT LIBOMP_HAVE_QUAD_PRECISION))
----------------
As above, I would keep error checking next to the code that sets up the variable.
================
Comment at: runtime/CMakeLists.txt:320
@@ -478,23 +319,3 @@
if(${WINDOWS})
- # Windows based systems use CMAKE_ASM_MASM_COMPILER
- # The windows assembly files are in MASM format, and they require a tool that can handle MASM syntax (ml.exe or ml64.exe typically)
- enable_language(ASM_MASM)
- set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake/${CMAKE_ASM_MASM_COMPILER_ID} ${CMAKE_MODULE_PATH})
- find_file(assembler_specific_include_file_found AsmFlags.cmake ${CMAKE_MODULE_PATH})
- if(assembler_specific_include_file_found)
- include(AsmFlags)
- append_assembler_specific_asm_flags(ASM_FLAGS)
- else()
- warning_say("Could not find cmake/${CMAKE_ASM_MASM_COMPILER_ID}/AsmFlags.cmake: will only use default flags")
- endif()
-else()
- # Unix (including Mac) based systems use CMAKE_ASM_COMPILER
- # Unix assembly files can be handled by compiler usually.
- find_file(assembler_specific_include_file_found AsmFlags.cmake ${CMAKE_MODULE_PATH})
- if(assembler_specific_include_file_found)
- include(AsmFlags)
- append_assembler_specific_asm_flags(ASM_FLAGS)
- else()
- warning_say("Could not find cmake/${CMAKE_ASM_COMPILER_ID}/AsmFlags.cmake: will only use default flags")
- endif()
+ set(LIBOMP_DEFAULT_LIB_NAME ${LIBOMP_DEFAULT_LIB_NAME}md)
endif()
----------------
This seems quite strange. Could you at least leave a comment about why this suffix is needed?
================
Comment at: runtime/cmake/LibompCheckLinkerFlag.cmake:14
@@ +13,3 @@
+# There is no real trivial way to do this in CMake, so we implement it here
+# this will have ${boolean} = TRUE if the flag succeeds, otherwise false.
+function(libomp_check_linker_flag flag boolean)
----------------
Should probably make 'TRUE' and 'false' agree on case.
================
Comment at: runtime/cmake/LibompCheckLinkerFlag.cmake:18-24
@@ +17,9 @@
+ set(retval TRUE)
+ set(library_source
+ "int foo(int a) { return a*a; }")
+ set(cmake_source
+ "cmake_minimum_required(VERSION 2.8)
+ project(foo C)
+ set(CMAKE_SHARED_LINKER_FLAGS \"${flag}\")
+ add_library(foo SHARED src_to_link.c)")
+ set(failed_regexes "[Ee]rror;[Uu]nknown;[Ss]kipping;LINK : warning")
----------------
I find the indent here hard to use. Maybe use a more conventional indent?
================
Comment at: runtime/cmake/LibompDefinitions.cmake:12
@@ +11,3 @@
+
+function(libomp_get_definitions_flags cppflags)
+ set(cppflags_local)
----------------
Rather than this, wouldn't it be better to use a config.h.cmake and process it into a config.h the way LLVM and Clang do? It might even be less code altogether.
================
Comment at: runtime/cmake/LibompHandleFlags.cmake:44
@@ +43,3 @@
+ if(${IA32})
+ libomp_append(flags_local -m32 LIBOMP_HAVE_M32_FLAG)
+ libomp_append(flags_local /arch:SSE2 LIBOMP_HAVE_ARCH_SSE2_FLAG)
----------------
Why do you want to blindly pass -m32 when it is supported? Seems odd that this is the only predicate..
================
Comment at: runtime/cmake/LibompHandleFlags.cmake:52-55
@@ +51,6 @@
+ endif()
+ libomp_append(flags_local -msse2 LIBOMP_HAVE_MSSE2_FLAG)
+ if(NOT LIBOMP_HAVE_MSSE2_FLAG)
+ libomp_append(flags_local -msse LIBOMP_HAVE_MSSE_FLAG)
+ endif()
+ libomp_append(flags_local /Oy- LIBOMP_HAVE_OY__FLAG)
----------------
Any chance we could just use '-march=native' or something?
================
Comment at: runtime/cmake/LibompHandleFlags.cmake:58
@@ +57,3 @@
+ libomp_append(flags_local /Qsafeseh LIBOMP_HAVE_QSAFESEH_FLAG)
+ libomp_append(flags_local -falign-stack=maintain-16-byte LIBOMP_HAVE_FALIGN_STACK_FLAG)
+ elseif(${MIC})
----------------
This is really concerning. It's an ABI-impacting flag that isn't supported by Clang at all... Not really sure what to do about it though.
================
Comment at: runtime/cmake/LibompSourceFiles.cmake:12-13
@@ +11,4 @@
+
+# files are relative to the src/ directory
+function(libomp_get_cfiles cfiles)
+ set(cfiles_local)
----------------
This doesn't seem right to me.
We shouldn't have the list of files stored far away from the actual files. The CMakeLists.txt in runtime/src should just pass the source files some function that builds the runtime.
================
Comment at: runtime/src/CMakeLists.txt:12-14
@@ -11,45 +11,5 @@
-####################
-# --- Create all ---
-add_custom_target(libomp-lib ALL DEPENDS omp)
-if(${LIBOMP_FORTRAN_MODULES})
- add_custom_target(libomp-mod ALL DEPENDS ${export_mod_files})
-endif()
-
-#############################
-# --- Create Common Files ---
-add_custom_target(libomp-common ALL DEPENDS ${export_cmn_files})
-add_custom_target(libomp-clean-common COMMAND ${CMAKE_COMMAND} -E remove -f ${export_cmn_files})
-
-# --- Put headers in convenient locations post build ---
-if(${LIBOMP_COPY_EXPORTS})
- add_custom_command(TARGET common POST_BUILD
- COMMAND ${CMAKE_COMMAND} -E make_directory ${export_cmn_dir}
- COMMAND ${CMAKE_COMMAND} -E copy omp.h ${export_cmn_dir}
- COMMAND ${CMAKE_COMMAND} -E copy omp_lib.h ${export_cmn_dir}
- COMMAND ${CMAKE_COMMAND} -E copy omp_lib.f ${export_cmn_dir}
- COMMAND ${CMAKE_COMMAND} -E copy omp_lib.f90 ${export_cmn_dir}
- )
- if(${LIBOMP_OMPT_SUPPORT})
- add_custom_command(TARGET common POST_BUILD
- COMMAND ${CMAKE_COMMAND} -E copy ompt.h ${export_cmn_dir}
- )
- endif()
- if(${LIBOMP_FORTRAN_MODULES})
- add_custom_command(TARGET mod POST_BUILD
- COMMAND ${CMAKE_COMMAND} -E make_directory ${export_mod_dir}
- COMMAND ${CMAKE_COMMAND} -E copy omp_lib.mod ${export_mod_dir}
- COMMAND ${CMAKE_COMMAND} -E copy omp_lib_kinds.mod ${export_mod_dir}
- )
- endif()
-endif()
-
-######################################################
-# --- Build the main library ---
-# $(lib_file) <== Main library file to create
-
-# preprocessor flags (-D definitions and -I includes)
-# Grab environment variable CPPFLAGS and append those to definitions
-set(include_dirs ${CMAKE_CURRENT_BINARY_DIR} ${src_dir} ${src_dir}/i18n ${inc_dir} ${src_dir}/thirdparty/ittnotify)
-include_directories(${include_dirs})
+include(LibompHandleFlags)
+include(LibompDefinitions)
+include(LibompSourceFiles)
----------------
I would somewhat expect the top-level CMake file to handle the inclusion and setup of these facilities, and this one to just use them.
================
Comment at: runtime/src/CMakeLists.txt:48
@@ -64,2 +47,3 @@
endif()
+set_source_files_properties(kmp_version.c PROPERTIES COMPILE_DEFINITIONS "_KMP_BUILD_TIME=\"\\\"${LIBOMP_DATE}\\\"\"")
----------------
Rather than any support for extracting the date from the build scripts, if this is ever desired, you should just use __DATE__ rather than this.
================
Comment at: runtime/src/CMakeLists.txt:151-154
@@ -155,31 +150,6 @@
-######################################################
-# Windows specific build rules
-if(${WINDOWS})
-
- # --- Create $(imp_file) ---
- # This file is first created in the unstripped/${lib_file} creation step.
- # It is then "re-linked" to include kmp_import.c which prevents linking of both Visual Studio OpenMP and newly built OpenMP
- if(NOT "${imp_file}" STREQUAL "")
- set(generated_import_file ${lib_file}${lib})
- add_library(ompimp STATIC ${generated_import_file} ${imp_src_files})
- set_source_files_properties(${generated_import_file} PROPERTIES GENERATED TRUE EXTERNAL_OBJECT TRUE)
- set_target_properties(ompimp PROPERTIES
- PREFIX "" SUFFIX ""
- OUTPUT_NAME "${imp_file}"
- STATIC_LIBRARY_FLAGS "${AR_FLAGS}"
- LINKER_LANGUAGE C
- SKIP_BUILD_RPATH true
- )
- add_custom_command(TARGET ompimp PRE_BUILD COMMAND ${CMAKE_COMMAND} -E remove -f ${imp_file})
- add_dependencies(ompimp omp)
- get_target_property(LIBOMPIMP_OUTPUT_DIRECTORY ompimp ARCHIVE_OUTPUT_DIRECTORY)
- if(NOT LIBOMPIMP_OUTPUT_DIRECTORY)
- set(LIBOMPIMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
- endif()
- if(${LIBOMP_COPY_EXPORTS})
- add_custom_command(TARGET ompimp POST_BUILD
- COMMAND ${CMAKE_COMMAND} -E make_directory ${export_lib_dir}
- COMMAND ${CMAKE_COMMAND} -E copy ${LIBOMPIMP_OUTPUT_DIRECTORY}/${imp_file} ${export_lib_dir}
- )
- endif()
+# Using expand-vars.pl to generate files
+# - 'file' is generated using expand-vars.pl and 'file'.var
+# - Any .var file should use this recipe
+# TODO: Use CMake's configure_file() instead
+macro(libomp_expand_vars_recipe file_dir filename)
----------------
This needs to move to the top so that its clear there are generated files prior to trying to compile them.
http://reviews.llvm.org/D10656
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the Openmp-commits
mailing list