[PATCH] D30043: [Polly][Cmake] Optionally use a system isl version.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thanks for the patch.

I did not understand the argument of not using a `FindISL.cmake`. You can have a FindISL.cmake and use FindPkgConfig in there? Once we bump to cmake 3.6 we can switch to using IMPORTED_TARGET in there and remove the manual creation of an imported target.



================
Comment at: CMakeLists.txt:172
+  foreach (incl IN LISTS SYSTEM_ISL_INCLUDE_DIRS)
+    set_property(TARGET SystemISL APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${incl})
+  endforeach()
----------------
`target_include_directories`?


================
Comment at: CMakeLists.txt:185
+  foreach (opt IN LISTS SYSTEM_ISL_CFLAGS SYSTEM_CFLAGS_OTHER)
+    set_property(TARGET SystemISL APPEND PROPERTY INTERFACE_COMPILE_OPTIONS ${opt})
+  endforeach()
----------------
`target_compile_options`?


================
Comment at: CMakeLists.txt:205-208
+  include_directories(
+  ${CMAKE_CURRENT_BINARY_DIR}/lib/External/isl/include
+  ${CMAKE_CURRENT_SOURCE_DIR}/lib/External/isl/include
+  )
----------------
I'd prefer to declare a variable `ISL_INCLUDE_DIRS` that contains the required directories and is set by the `POLLY_USE_EXTERNAL_ISL`-handling mechanism to the directories of choice. It avoids having extra cases for `POLLY_USE_EXTERNAL_ISL` whenever it is used.

Would using `target_include_directories(PollyISL INTERFACE ...)` make this unnecessary?


================
Comment at: lib/External/CMakeLists.txt:3
+if (NOT POLLY_USE_EXTERNAL_ISL)
 set(ISL_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/isl")
 set(ISL_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/isl")
----------------
Indention


https://reviews.llvm.org/D30043