[PATCH] LLVM OpenMP CMake Overhaul
Jonathan Peyton
jonathan.l.peyton at intel.com
Fri Jun 26 13:07:39 PDT 2015
Commented on inline suggestions.
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)")
----------------
emaste wrote:
> chandlerc wrote:
> > CMake already provides variables for querying the OS - why define your own?
> And FreeBSD is missing if we do define our own; the current convention of lin == Unix-like that is not Apple is confusing.
Good points from both of you. In the second version of this patch I have removed LIBOMP_OS, The build systems relies on the CMake variables WIN32 and APPLE exclusively. I've also added a small section in config-ix.cmake where I define LIBOMP_PERL_SCRIPT_OS based on WIN32 and APPLE. I put a comment there explaining the reasoning behind it and will explain here as well. The perl scripts (which I'm trying to get rid of) have deeply embedded in them an OS check which expects certain OS's. If the user passes --os=lin to the perl script, it checks that lin is valid (which it already is) and will run without problem and use Unix tools under the hood. It is just plain easier and more portable to have Non-Windows and Non-Apple operating systems be considered "lin" in this context (Most unix flavors). This way people on NetBSD for example aren't going to have to dig through the opaque perl module files in runtime/tools/lib and add NetBSD to it.
================
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)")
----------------
chandlerc wrote:
> 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.
Jim has already given the details on this topic. I haven't addressed any problems with the Fortran Module files in the second iteration, but they aren't created by default (meaning users don't need a Fortran compiler) and shouldn't be a big problem to remove if that is what we decide on.
================
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?")
----------------
chandlerc wrote:
> Why provide cache settings for any of these? It seems like unnecessary complexity.
The LIBOMP_MICRO_TESTS is absolutely unnecessary. I've removed the others as well in the second iteration. An argument for the LIBOMP_TEST_* cache variables is if a user is having troubles with one of them, then he/she can just turn it off and not have to think about it again. I have removed them though.
================
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?")
----------------
chandlerc wrote:
> Should this really be true? Is this important to support? There isn't much context here about why this is needed.
I've added a comment and TODO here in the second iteration. The testsuite/ directory expects the library to be placed in the exports/ directory so it can grab it from there. I know this is bad, and I'm trying to get my hands around the testsuite/ to convert it to llvm-lit based testing.
================
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()
----------------
chandlerc wrote:
> 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?
I've moved this section under LIBOMP_STANDALONE_BUILD to help reinforce that it is for standalone builds.
OpenMP is often included in HPC codes and I feel the default library should be optimized for performance.
I also feel like most users outside LLVM are going to do this: $ cmake [maybe set compilers] ..
And I want that to build the optimized library.
================
Comment at: runtime/CMakeLists.txt:144
@@ +143,3 @@
+endif()
+string(TOLOWER "${CMAKE_BUILD_TYPE}" libomp_build_type_lowercase)
+
----------------
chandlerc wrote:
> I would suggest creating this below, where you actually need and use it.
Done in second iteration.
================
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
----------------
chandlerc wrote:
> 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.
Done in second iteration.
================
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")
----------------
chandlerc wrote:
> At this point we have three different ways to detect Linux vs. Mac. =/ This doesn't seem good at all.
This OS detection problem is addressed with my first comment.
================
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})
----------------
chandlerc wrote:
> I'm curious why these variables are needed?
These are for convenience. They give me absolute source paths no matter what CMakeLists.txt file I'm in or *.cmake file I'm in. The LIBOMP_BASE_DIR variable in particular is popular with most projects because it says "I know exactly where I am. I'm at the very top directory of this project", and then all files can use this variable to navigate.
================
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?")
+
----------------
chandlerc wrote:
> This seems really strange to be a CMake thing and not something in the source code.
This was requested by ScaleMP. I'm not inclined to remove it since it is off by default.
================
Comment at: runtime/CMakeLists.txt:294
@@ +293,3 @@
+
+# Error checking the configuration
+if(LIBOMP_USE_QUAD_PRECISION AND (NOT LIBOMP_HAVE_QUAD_PRECISION))
----------------
chandlerc wrote:
> As above, I would keep error checking next to the code that sets up the variable.
Done in second iteration.
================
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()
----------------
chandlerc wrote:
> This seems quite strange. Could you at least leave a comment about why this suffix is needed?
Well, when this library was first put inside LLVM, we wanted the name to be exactly like Intel's but since that has changed, I have removed this suffix as well. Now Windows will build libomp.dll and libomp.lib.
================
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)
----------------
chandlerc wrote:
> Should probably make 'TRUE' and 'false' agree on case.
Done in second iteration.
================
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")
----------------
chandlerc wrote:
> I find the indent here hard to use. Maybe use a more conventional indent?
I've indented this over four spaces in the second iteration.
================
Comment at: runtime/cmake/LibompDefinitions.cmake:12
@@ +11,3 @@
+
+function(libomp_get_definitions_flags cppflags)
+ set(cppflags_local)
----------------
chandlerc wrote:
> 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.
Yes, I completely agree. I wanted to tackle this when I remove expand-vars.pl (which is just a perl based configure_file()-like script)
================
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)
----------------
chandlerc wrote:
> jhowarth wrote:
> > chandlerc wrote:
> > > Why do you want to blindly pass -m32 when it is supported? Seems odd that this is the only predicate..
> > This may be for the building the fat libomp.dyib on x86_64 darwin (containing both i386 and x86_64 code) which requires -m32 to be explicitly passed to the native x86_64 compiler.
> Sure, I know that this is what -m32 does, but it doesn't make sense to *blindly* pass it. I feel like we should be very specifically testing for targeting i386 with a x86_64 toolchain first.
Compiler-rt blindly passes it in if it detects the __i386__ symbol and NOT MSVC so I thought it would be ok if we are targeting 32-bit x86.
After looking over libcxx and llvm, I see that they at least do this:
if( CMAKE_SIZEOF_VOID_P EQUAL 8 AND NOT WIN32 )
(logic to include m32)
endif()
Although, they blindly assume the -m32 flag exists (which may not be a huge assumption).
I can add this wrapper in the third iteration.
================
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)
----------------
chandlerc wrote:
> Any chance we could just use '-march=native' or something?
I've removed the checks for /arch:SSE and /arch:IA32 and -msse and kept the msse2 check. The second iteration now will just append msse2 if they have it. -march=native should be something the user specifies.
================
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})
----------------
chandlerc wrote:
> 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.
Here is some info about it: https://software.intel.com/sites/products/documentation/doclib/iss/2013/compiler/cpp-lin/GUID-8EB32E6C-EA4B-4D2A-B4BF-9FA2774C505A.htm
It is an Intel compiler specific flag that was inserted to maintain binary compatibility when GCC changed their ABI on 32-bit x86 which started making the assumption that stacks were 16 byte aligned to aid SSE instructions. It is supposed to be the most backwards compatible flag.
================
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)
----------------
chandlerc wrote:
> 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.
Can I just move this logic to runtime/src/CMakeLists.txt and delete LibompSourceFiles?
================
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)
----------------
chandlerc wrote:
> 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.
Moved to top-level CMakeLists.txt file in second iteration.
================
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}\\\"\"")
----------------
chandlerc wrote:
> 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.
I'll have to change this in the third iteration. FWIW, CMake offers string(TIMESTAMP ...) in v2.8.11 so someone wanted it :)
================
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)
----------------
chandlerc wrote:
> This needs to move to the top so that its clear there are generated files prior to trying to compile them.
Moved to top in second iteration.
http://reviews.llvm.org/D10656
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list