This is an archive of the discontinued LLVM Phabricator instance.

[Polly][Cmake] Generate a PollyConfig.cmake
ClosedPublic

Authored by philip.pfaffe on Mar 1 2017, 4:03 AM.

Details

Summary

Generate a PollyConfig.cmake for use with Cmake's find_package in out-of-tree projects.

This patch is based on Meinersbur's template, including some fixes and most importantly the appropriate ISL target.

Diff Detail

Repository
rL LLVM

Event Timeline

philip.pfaffe created this revision.Mar 1 2017, 4:03 AM
Meinersbur edited edge metadata.Mar 2 2017, 10:45 AM

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

cmake/CMakeLists.txt
46 ↗(On Diff #90158)

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.

cmake/PollyConfig.cmake.in
4 ↗(On Diff #90158)

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.

9 ↗(On Diff #90158)

Who is setting POLLY_CONFIG_INCLUDE_DIRS? Its plural name indicates it's a list, so shouldn't be surrounded by quotes.

10 ↗(On Diff #90158)

In standalone (not built together with LLVM) builds, the LIBRARY_DIRS of Polly will be different from that of LLVM.

11 ↗(On Diff #90158)

PollyPPCG should depend on POLLY_ENABLE_GPGPU_CODEGEN

12 ↗(On Diff #90158)

Terminating @ missing?

19–22 ↗(On Diff #90158)

Existence of this target should also depend on POLLY_ENABLE_GPGPU_CODEGEN.

You seem to have decided against using it. Can you share why?

I didn't go with export() because it solves only half the problem. The exported targets are only valid for the build tree. The equivalent for the install tree would be install(EXPORT) together with the appropriate per-target install options. I don't think the second path is feasible for Polly, because it requires invasive changes to LLVM's cmake setup. Thus, I'd have to manually export the targets for the install tree anyways. I still could've used export() for the build tree, but I prefer the symmetry of my solution.

Updates wrt. to the inline comments coming soon.

OK, thanks for explaining.

Addressed inline comments and a couple of extra issues I found, specifically the missing IMPORTED_LOCATION settings.

philip.pfaffe marked 7 inline comments as done.Mar 6 2017, 7:58 AM

Thanks for this update. Here are some more comments.

cmake/CMakeLists.txt
13 ↗(On Diff #90702)

Did you mean

list(APPEND POLLY_CONFIG_EXPORTED_TARGETS PollyPPCG)

?

44 ↗(On Diff #90702)

Clang uses

set(clang_cmake_builddir "${CMAKE_BINARY_DIR}/${CLANG_INSTALL_PACKAGE_DIR}")

(CMAKE_BINARY_DIR instead of POLLY_BINARY_DIR). The effect is that the PollyConfig.cmake lands in <build dir>/lib/polly/PollyConfig.cmake, not in <build dir>/tools/polly/lib/cmake/PollyConfig.cmake as done currently. I think we should do the same. I think the idea is that all the *Config.cmake for the build dir can be found at a central place. We should do the same.

45 ↗(On Diff #90702)

Did you mean

set(POLLY_CONFIG_INCLUDE_DIRS ${ISL_INCLUDE_DIRS})

?

65–67 ↗(On Diff #90702)

On Windows, this generates several errors:

CMake Error at tools/polly/cmake/CMakeLists.txt:63 (file):
  Error evaluating generator expression:

    $<TARGET_FILE:LLVMPolly>

  Target "LLVMPolly" is not an executable or library.

(same for line 109)
Because LLVMPolly is only a dummy target on Windows.

For the other targets these errors show up:

CMake Error in tools/polly/cmake/CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    C:/Users/Meinersbur/build/llvm/vc14/tools/polly/lib/cmake/polly/PollyExports.cmake

with multi-configuration generators such as Visual Studio.

I don't really care about having this generated on Windows, but there shouldn't be errors. Can you just skip this in these cases? I suggest:

if (NOT CMAKE_CONFIGURATION_TYPES AND NOT MSVC)
  add_subdirectory(cmake)
endif ()

(this also excludes XCode)

Addressed inline comments.

This patch now generates one Exports file per Configuration and should now also work on Windows.

philip.pfaffe marked 4 inline comments as done.Mar 8 2017, 10:22 AM
Meinersbur accepted this revision.Mar 9 2017, 8:16 AM

Thanks for making it work under Windows. Could you just remove the debugging line if it is one (or let me do it), then I'll commit.

cmake/CMakeLists.txt
56 ↗(On Diff #91046)

Is this a leftover from debugging?

This revision is now accepted and ready to land.Mar 9 2017, 8:16 AM

Removed the Debug output. My bad!

philip.pfaffe marked an inline comment as done.Mar 9 2017, 8:36 AM
This revision was automatically updated to reflect the committed changes.

Thanks for all the effort!