[PATCH] D72418: [Flang] add some cmake code to allow for out-of-tree building of MLIR and LLVM

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar marked an inline comment as done.
jpienaar added a comment.
Herald added subscribers: liufengdb, aartbik.

Nice, seems to match what clang does here too so that's good to keep it in sync



================
Comment at: mlir/CMakeLists.txt:11-15
 function(mlir_tablegen ofn)
   tablegen(MLIR ${ARGV} "-I${MLIR_MAIN_SRC_DIR}" "-I${MLIR_INCLUDE_DIR}")
   set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT} ${CMAKE_CURRENT_BINARY_DIR}/${ofn}
       PARENT_SCOPE)
 endfunction()
----------------
Cant this be removed now?


================
Comment at: mlir/CMakeLists.txt:44-64
 # TODO: This is to handle the current static registration, but should be
 # factored out a bit.
 function(whole_archive_link target)
   if("${CMAKE_SYSTEM_NAME}" STREQUAL "Darwin")
     set(link_flags "-L${CMAKE_BINARY_DIR}/lib ")
     FOREACH(LIB ${ARGN})
       string(CONCAT link_flags ${link_flags} "-Wl,-force_load ${CMAKE_BINARY_DIR}/lib/lib${LIB}.a ")
----------------
Can this be removed now?


================
Comment at: mlir/cmake/modules/CMakeLists.txt:1-3
+# Generate a list of CMake library targets so that other CMake projects can
+# link against them. LLVM calls its version of this file LLVMExports.cmake, but
+# the usual CMake convention seems to be ${Project}Targets.cmake.
----------------
I was going to comment "What does clang do?" and then I saw they have this exact same comment ;-)


================
Comment at: mlir/cmake/modules/CMakeLists.txt:28-29
+set(MLIR_CONFIG_CODE "
+# Compute the installation prefix from this LLVMConfig.cmake file location.
+get_filename_component(MLIR_INSTALL_PREFIX \"\${CMAKE_CURRENT_LIST_FILE}\" PATH)")
+# Construct the proper number of get_filename_component(... PATH)
----------------
Nit: could you indent these to show the nesting?


================
Comment at: mlir/cmake/modules/CMakeLists.txt:35
+  set(MLIR_CONFIG_CODE "${MLIR_CONFIG_CODE}
+get_filename_component(MLIR_INSTALL_PREFIX \"\${MLIR_INSTALL_PREFIX}\" PATH)")
+endforeach(p)
----------------
Same re indent


================
Comment at: mlir/cmake/modules/CMakeLists.txt:43-46
+#configure_file(
+#  ${CMAKE_CURRENT_SOURCE_DIR}/MlirConfig.cmake.in
+#  ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/MlirConfig.cmake
+#  @ONLY)
----------------
Leftover from testing?


================
Comment at: mlir/cmake/modules/CMakeLists.txt:51
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  get_property(mlir_has_exports GLOBAL PROPERTY MLIR_HAS_EXPORTS)
----------------
Could you add a comment for this section?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72418/new/

https://reviews.llvm.org/D72418