[PATCH] D29931: [Cmake] Install the isl headers into the install tree

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a reviewer: grosser.
Meinersbur added a comment.

I expected it would "just work" because eg. `ScopInfo.h` uses

  #include "isl/ctx.h"

and therefore already find `ctx.h` in the `isl` sibling directory. (#include by quotes first searches relative to the directory of the current file). Unfortunately we have headers in subdirectories such as `Support/SCEVAffinator.h` which also `#include "isl/ctx.h"` but does not find it.

I would have liked if we would have flattened the header directory structure instead as I couldn't identify a pattern when a header it put in a subdirectory and when it does not. Unfortunately the isl headers themselves use

  #include <isl/ctx.h>

and therefore  do not find their sibling files without a compiler flag. Even worse, while `ScopInfo.h` finds `isl/ctx.h`, the others will fall back to the system isl (if available) and give us a mix of Polly's isl headers and system isl headers. This might not be too much of a problem because I do not expect many compatibility issues between them (as long as we do not use any C++ bindings)

Could you also write a few lines into the documentation about how to use it? For instance, it is also important to link against libPollyISL, not libisl.so.

You are right in that a `PollyConfig.cmake` would be a solution that could handle these things. However, this requires cmake as your build system.



================
Comment at: lib/External/CMakeLists.txt:266
+
+# TODO: use system isl instead
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
----------------
"//optionally// use system isl instead"?


================
Comment at: lib/External/CMakeLists.txt:268-269
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  set(POLLY_ISL_HEADER_PREFIX "include/polly" CACHE STRING
+    "Install prefix for the isl headers")
+  mark_as_advanced(POLLY_ISL_HEADER_PREFIX)
----------------
Does it make sense to make this configurable? What would be the use case for this?

The cmake doc for `install(DESTINATION)`mentions it can be either absolute or relative to CMAKE_INSTALL_PREFIX.  Could you mention that in the description and/or rename it to `POLLY_ISL_HEADER_INSTALL_PREFIX`?


https://reviews.llvm.org/D29931