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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz marked 4 inline comments as done.
schweitz added inline comments.


================
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()
----------------
jpienaar wrote:
> Cant this be removed now?
Possibly. We can give it a try.


================
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)
----------------
jpienaar wrote:
> Nit: could you indent these to show the nesting?
Sure. :)


================
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)
----------------
jpienaar wrote:
> Leftover from testing?
This was leftover from cobbling things together from other LLVM projects. Other projects seem to setup this XXXConfig.cmake file, but I found it wasn't needed here.

We can remove these commented out lines, if you prefer.  I left them with the chance someone who knew CMake better had good reasons to build out an MLIRConfig.cmake rule.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72418