[PATCH] D30495: [Polly][Cmake] Generate a PollyConfig.cmake

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thank you for picking up and continue my PollyConfig patch. I stopped when I discovered that I could generate the `IMPORTED` libraries using the `export` <https://cmake.org/cmake/help/v3.7/command/export.html> command and couldn't decide whether it was better. You seem to have decided against using it. Can you share why?



================
Comment at: cmake/CMakeLists.txt:46
+if (POLLY_BUNDLED_ISL)
+  set(ISL_INCLUDE_DIRS "${POLLY_INSTALL_PREFIX}/include/polly")
+endif()
----------------
This shouldn't overwrite `ISL_INCLUDE_DIRS`, even if it is changed just in this CMakeLists.txt. Just add it to `POLLY_CONFIG_INCLUDE_DIRS`.


================
Comment at: cmake/PollyConfig.cmake.in:4
+find_package(LLVM REQUIRED CONFIG
+             HINTS "@POLLY_CONFIG_LLVM_CMAKE_DIR@")
+
----------------
I tried a standalone build where `POLLY_CONFIG_LLVM_CMAKE_DIR` turned out to be `/root/install/polly/release/lib/cmake/llvm`, but should be `/root/install/llvm/release/lib/cmake/llvm` (`${LLVM_INSTALL_ROOT}/${LLVM_INSTALL_PACKAGE_DIR}`)
(`-DCMAKE_INSTALL_PREFIX=/root/install/llvm/release` for llvm w/o Polly and 
`-DCMAKE_INSTALL_PREFIX=/root/install/polly/release` for Polly). 

`llvm_cmake_builddir` might need to be set to something else in that case. I think I copied that part from clang, they might have it wrong too.


================
Comment at: cmake/PollyConfig.cmake.in:9
+set(Polly_DEFINITIONS ${LLVM_DEFINITIONS})
+set(Polly_INCLUDE_DIRS "@POLLY_CONFIG_INCLUDE_DIRS@" ${LLVM_INCLUDE_DIRS} @ISL_INCLUDE_DIRS@)
+set(Polly_LIBRARY_DIRS ${LLVM_LIBRARY_DIRS})
----------------
Who is setting `POLLY_CONFIG_INCLUDE_DIRS`? Its plural name indicates it's a list, so shouldn't be surrounded by quotes.


================
Comment at: cmake/PollyConfig.cmake.in:10
+set(Polly_INCLUDE_DIRS "@POLLY_CONFIG_INCLUDE_DIRS@" ${LLVM_INCLUDE_DIRS} @ISL_INCLUDE_DIRS@)
+set(Polly_LIBRARY_DIRS ${LLVM_LIBRARY_DIRS})
+set(Polly_LIBRARIES ${LLVM_LIBRARIES} @ISL_TARGET@ PollyPPCG Polly)
----------------
In standalone (not built together with LLVM) builds, the `LIBRARY_DIRS` of Polly will be different from that of LLVM.


================
Comment at: cmake/PollyConfig.cmake.in:11
+set(Polly_LIBRARY_DIRS ${LLVM_LIBRARY_DIRS})
+set(Polly_LIBRARIES ${LLVM_LIBRARIES} @ISL_TARGET@ PollyPPCG Polly)
+set(Polly_BUNDLED_ISL @POLLY_BUNDLED_ISL)
----------------
`PollyPPCG` should depend on `POLLY_ENABLE_GPGPU_CODEGEN`


================
Comment at: cmake/PollyConfig.cmake.in:12
+set(Polly_LIBRARIES ${LLVM_LIBRARIES} @ISL_TARGET@ PollyPPCG Polly)
+set(Polly_BUNDLED_ISL @POLLY_BUNDLED_ISL)
+
----------------
Terminating `@` missing?


================
Comment at: cmake/PollyConfig.cmake.in:19-22
+if (NOT TARGET PollyPPCG)
+  add_library(PollyPPCG @POLLY_CONFIG_LIBKIND@ IMPORTED)
+  target_link_libraries(PollyPPCG INTERFACE @ISL_TARGET@)
+endif()
----------------
Existence of this target should also depend on `POLLY_ENABLE_GPGPU_CODEGEN`.


Repository:
  rL LLVM

https://reviews.llvm.org/D30495