[PATCH] D10656: LLVM OpenMP CMake Overhaul
Chandler Carruth
chandlerc at gmail.com
Tue Jul 14 19:08:23 PDT 2015
chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.
First and foremost, thank you again for working on this.
I want to call out one thing that was very helpful in case it went unnoticed. Your replies of things that you're planning to do in subsequent iterations and your replies giving context as to *why* things are the way they are. Totally useful.
Also, I appreciate the added comments archiving context for the next reader. I don't want you to feel like that was wasted time, it helps tremendously.
At this point, I think you should commit this after addressing the remaining (minor) comments from Ed Maste and some of the simple things below. This is *significantly* better than the starting point, and I'm interested in letting you get to the next iteration.
Once it lands, I'll also test out using it, and look to see if I spot any actual functional issues.
================
Comment at: runtime/CMakeLists.txt:34
@@ +33,3 @@
+if(${LIBOMP_STANDALONE_BUILD})
+ # If adding a new architecture, take a look at cmake/LibompGetArchitecture.cmake
+ libomp_get_architecture(LIBOMP_DETECTED_ARCH)
----------------
Maybe just convert everything to 2-space indent? That would be most consistent with the rest of LLVM, but maybe not the openmp runtime. But within the else() side of this if() you have 2-space indent creeping in already. I think having them be consistent will be really useful for readability.
================
Comment at: runtime/CMakeLists.txt:196
@@ +195,3 @@
+# Enabling Fortran if it is needed
+# Enable MASM Compiler if it is needed (Windows only)
+if(${LIBOMP_FORTRAN_MODULES})
----------------
This comment seems now out of place.
================
Comment at: runtime/CMakeLists.txt:268-274
@@ -410,142 +267,9 @@
# Setting final library name
-set(lib_item "libomp")
+set(LIBOMP_DEFAULT_LIB_NAME libomp)
if(${PROFILE_LIBRARY})
- set(lib_item "${lib_item}prof")
+ set(LIBOMP_DEFAULT_LIB_NAME ${LIBOMP_DEFAULT_LIB_NAME}prof)
endif()
if(${STUBS_LIBRARY})
- set(lib_item "${lib_item}stubs")
-endif()
-if(${WINDOWS})
- set(lib_item "${lib_item}md")
-endif()
-set(LIBOMP_LIB_NAME "${lib_item}" CACHE STRING "OMP library name")
-set(lib_ext "${dll}")
-# ${lib_file} is real library name:
-# libomp.so for Linux
-# libomp.dylib for Mac
-# libompmd.dll for Windows
-set(lib_file "${LIBOMP_LIB_NAME}${lib_ext}")
-
-########################################
-# Setting export file names
-if(${WINDOWS})
- set(imp_file "${lib_item}${lib}") # this is exported (libomp.lib)
- set(def_file "${lib_item}.def") # this is not exported
- if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug" OR
- "${CMAKE_BUILD_TYPE}" STREQUAL "RelWithDebInfo" OR
- ${LIBOMP_USE_BUILDPL_RULES})
- set(pdb_file "${lib_file}.pdb") # this is exported if it exists (libompmd.dll.pdb)
- endif()
-endif()
-set(export_lib_files "${lib_file}" "${imp_file}" "${pdb_file}")
-set(export_mod_files "omp_lib.mod" "omp_lib_kinds.mod")
-set(export_cmn_files "omp.h" "omp_lib.h" "omp_lib.f" "omp_lib.f90")
-
-if(${LIBOMP_OMPT_SUPPORT})
- set(export_cmn_files ${export_cmn_files} "ompt.h")
-endif()
-
-if("${export_lib_fat_dir}")
- set(export_lib_fat_files "${lib_file}" "${imp_file}")
-endif()
-
-#########################
-# Getting legal type/arch
-set_legal_type(legal_type)
-set_legal_arch(legal_arch)
-
-#################################################
-# Preprocessor Definitions (cmake/Definitions.cmake)
-# Preprocessor Includes
-# Compiler (C/C++) Flags (cmake/CommonFlags.cmake)
-# Assembler Flags (cmake/CommonFlags.cmake)
-# Fortran Flags (cmake/CommonFlags.cmake)
-# Linker Flags (cmake/CommonFlags.cmake)
-# Archiver Flags (cmake/CommonFlags.cmake)
-# Helper Perl Script Flags (cmake/PerlFlags.cmake)
-# * Inside the cmake/CommonFlags.cmake file, the LIBOMP_*FLAGS are added.
-# * Cannot use CMAKE_*_FLAGS directly because -x c++ is put in the linker command and mangles the linking phase.
-
-# Grab compiler-dependent flags
-# Cmake will look for cmake/${CMAKE_C_COMPILER_ID}/CFlags.cmake to append additional c, cxx, and linker flags.
-set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake/${CMAKE_C_COMPILER_ID} ${CMAKE_MODULE_PATH})
-find_file(compiler_specific_include_file_found CFlags.cmake ${CMAKE_MODULE_PATH})
-if(compiler_specific_include_file_found)
- include(CFlags) # COMPILER_SUPPORTS_QUAD_PRECISION changed in here
- append_compiler_specific_c_and_cxx_flags(C_FLAGS CXX_FLAGS)
- append_compiler_specific_linker_flags(LD_FLAGS LD_LIB_FLAGS)
-else()
- warning_say("Could not find cmake/${CMAKE_C_COMPILER_ID}/CFlags.cmake: will only use default flags")
-endif()
-# Grab assembler-dependent flags
-# CMake will look for cmake/${CMAKE_ASM_COMPILER_ID}/AsmFlags.cmake to append additional assembler flags.
-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()
-endif()
-
-# Grab all the compiler-independent flags
-append_c_and_cxx_flags_common(C_FLAGS CXX_FLAGS)
-append_asm_flags_common(ASM_FLAGS)
-append_fort_flags_common(F_FLAGS)
-append_linker_flags_common(LD_FLAGS LD_LIB_FLAGS)
-append_archiver_flags_common(AR_FLAGS)
-append_cpp_flags(DEFINITIONS_FLAGS)
-
-# Setup the flags correctly for cmake (covert to string)
-# Pretty them up (STRIP any beginning and trailing whitespace)
-list_to_string("${DEFINITIONS_FLAGS}" DEFINITIONS_FLAGS)
-list_to_string("${C_FLAGS}" C_FLAGS)
-list_to_string("${CXX_FLAGS}" CXX_FLAGS)
-list_to_string("${ASM_FLAGS}" ASM_FLAGS)
-list_to_string("${LD_FLAGS}" LD_FLAGS)
-list_to_string("${LD_LIB_FLAGS}" LD_LIB_FLAGS)
-# Windows specific for creating import library
-list_to_string("${AR_FLAGS}" AR_FLAGS)
-string(STRIP "${DEFINITIONS_FLAGS}" DEFINITIONS_FLAGS)
-string(STRIP "${C_FLAGS}" C_FLAGS)
-string(STRIP "${CXX_FLAGS}" CXX_FLAGS)
-string(STRIP "${ASM_FLAGS}" ASM_FLAGS)
-string(STRIP "${LD_FLAGS}" LD_FLAGS)
-string(STRIP "${LD_LIB_FLAGS}" LD_LIB_FLAGS)
-# Windows specific for creating import library
-string(STRIP "${AR_FLAGS}" AR_FLAGS)
-
-# Grab the Perl flags
-set_ev_flags(ev_flags) # expand-vars.pl flags
-set_gd_flags(gd_flags) # generate-def.pl flags (Windows only)
-set(oa_opts "--os=${real_os}" "--arch=${LIBOMP_ARCH}") # sent to the perl scripts
-
-#########################################################
-# Getting correct source files (cmake/SourceFiles.cmake)
-set_c_files(lib_c_items)
-set_cpp_files(lib_cxx_items)
-set_asm_files(lib_asm_items)
-set_imp_c_files(imp_c_items) # Windows-specific
-set(lib_src_files "${lib_c_items}" "${lib_cxx_items}" "${lib_asm_items}")
-set(imp_src_files "${imp_c_items}")
-
-#####################################################################
-# Debug print outs. Will print "variable = ${variable}" if GLOBAL_DEBUG == 1
-if(GLOBAL_DEBUG)
- include(CMakePrintSystemInformation)
+ set(LIBOMP_DEFAULT_LIB_NAME ${LIBOMP_DEFAULT_LIB_NAME}stubs)
endif()
+set(LIBOMP_LIB_NAME ${LIBOMP_DEFAULT_LIB_NAME} CACHE STRING "Base OMP library name")
----------------
A question that isn't relevant for this patch, but I didn't want to forget.
Would it make sense to use '_prof' and '_stubs' so the names read a bit easier? Or are there users relying on these suffixes? Mostly curious.
Repository:
rL LLVM
http://reviews.llvm.org/D10656
More information about the llvm-commits
mailing list