[PATCH] D30043: [Polly][Cmake] Optionally use a system isl version.
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 20 09:49:16 PST 2017
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
More information about the llvm-commits
mailing list