[PATCH] D33299: [Polly][CMake] Use the CMake Package instead of llvm-config in out-of-tree builds

[Philip Pfaffe via Phabricator]

philip.pfaffe created this revision.
Herald added a subscriber: mgorny.

As of now, Polly uses llvm-config to set up LLVM dependencies in an out-of-tree build.

This is problematic for two reasons:

1. Right now, in-tree and out-of-tree builds in fact do different things. E.g., in an in-tree build, libPolly depends on a handful of LLVM libraries, while in an out-of-tree build it depends on all of them. This means that we often need to treat both paths seperately.
2. I'm specifically unhappy with the way libPolly is linked right now, because it just blindly links against all the LLVM libs. That doesn't make a lot of sense. For instance, one of these libs is LLVMTableGen, which contains a command line definition of a -o option. This means that I can not link an out-of-tree libPolly into a tool which might want to offer a -o option as well.

This patch (mostly) drop the use of llvm-config in favor of LLVMs exported cmake package. However, building Polly with unittests requires access to the gtest sources (in the LLVM source tree). If we're building against an LLVM installation, this source tree is unavailable and must specified. I'm using llvm-config to provide a default in this case.


https://reviews.llvm.org/D33299

Files:
CMakeLists.txt
lib/CMakeLists.txt

[Michael Kruse via Phabricator]

Meinersbur added a comment.

Can you rebase to current trunk? I'd like to try out by myself.

Are some of the condition the result of using the LLVMConfig.cmake from and install_prefix or build directory?



================
Comment at: CMakeLists.txt:24
+ if (LLVM_BUILD_MAIN_SRC_DIR)
+ set(LLVM_SOURCE_ROOT ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
+ else()
----------------
This is the same line in both cases?


================
Comment at: CMakeLists.txt:26
+ else()
+ execute_process(COMMAND "${LLVM_INSTALL_ROOT}/bin/llvm-config" --src-root
+ OUTPUT_VARIABLE MAIN_SRC_DIR
----------------
Still using `llvm-config`? If there is no `LLVM_BUILD_MAIN_SRC_DIR`, shouldn't the llvm-config also fail?


================
Comment at: CMakeLists.txt:35
if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
- add_library(gtest
- ${UNITTEST_DIR}/googletest/src/gtest-all.cc
- ${UNITTEST_DIR}/googlemock/src/gmock-all.cc
- )
- target_include_directories(gtest
- PUBLIC
- "${UNITTEST_DIR}/googletest/include"
- "${UNITTEST_DIR}/googlemock/include"
-
- PRIVATE
- "${UNITTEST_DIR}/googletest"
- "${UNITTEST_DIR}/googlemock"
- )
- target_link_libraries(gtest -lpthread)
-
- add_library(gtest_main ${UNITTEST_DIR}/UnitTestMain/TestMain.cpp)
- target_link_libraries(gtest_main gtest)
-
- set(POLLY_GTEST_AVAIL 1)
+ if (TARGET gtest)
+ set_target_properties(gtest
----------------
Under which conditions does LLVM not export a gtest target?


================
Comment at: lib/CMakeLists.txt:109
+ LLVMPasses
+ ${nvptx_libs}
+ # The libraries below are required for darwin: http://PR26392
----------------
Has this order change a reason?

You seem to have added `LLVMPasses` to your local changes.


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe marked an inline comment as done.
philip.pfaffe added inline comments.

================
Comment at: CMakeLists.txt:24
+ if (LLVM_BUILD_MAIN_SRC_DIR)
+ set(LLVM_SOURCE_ROOT ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
+ else()
----------------

Meinersbur wrote:
> This is the same line in both cases?

The former is a typo and should use LLVM_BUILD_MAIN_SRC_DIR.

================
Comment at: CMakeLists.txt:26
+ else()
+ execute_process(COMMAND "${LLVM_INSTALL_ROOT}/bin/llvm-config" --src-root
+ OUTPUT_VARIABLE MAIN_SRC_DIR
----------------

Meinersbur wrote:
> Still using `llvm-config`? If there is no `LLVM_BUILD_MAIN_SRC_DIR`, shouldn't the llvm-config also fail?

At this point, llvm-config is purely used to provide a default for the setting. I expect the result to be useful when you're building against the install tree of a local build. It won't be helpful if you're using, e.g., a binary distribution for instance.

Meinersbur wrote:
> Under which conditions does LLVM not export a gtest target?

The install tree doesn't contain gtest.

================
Comment at: lib/CMakeLists.txt:109
+ LLVMPasses
+ ${nvptx_libs}
+ # The libraries below are required for darwin: http://PR26392
----------------

Meinersbur wrote:
> Has this order change a reason?
>
> You seem to have added `LLVMPasses` to your local changes.

Yes, this one slipped through.


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe updated this revision to Diff 99701.

philip.pfaffe marked an inline comment as done.

philip.pfaffe added a comment.
Herald added a subscriber: bollu.

Rebase onto trunk.

- Fix typo
- Add comments about the existence of the gtest target

[Philip Pfaffe via Phabricator]

philip.pfaffe marked 3 inline comments as done.

https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser added a dependency: D33387: [Polly] [Docs] Use ReadTheDocs theme if available..

https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser removed a dependency: D33387: [Polly] [Docs] Use ReadTheDocs theme if available..

https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser added a dependent revision: D33387: [Polly] [Docs] Use ReadTheDocs theme if available..

https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser removed a dependent revision: D33387: [Polly] [Docs] Use ReadTheDocs theme if available..

https://reviews.llvm.org/D33299

[Michael Kruse via Phabricator]

Meinersbur added a comment.

My first tryout failed with this error:

CMake Error at /usr/share/llvm-3.6/cmake/AddLLVM.cmake:468 (set_output_directory):
set_output_directory Function invoked with incorrect arguments for function
named: set_output_directory
Call Stack (most recent call first):
/usr/share/llvm-3.6/cmake/AddLLVM.cmake:604 (add_llvm_executable)
unittests/CMakeLists.txt:9 (add_unittest)
unittests/Isl/CMakeLists.txt:1 (add_polly_unittest)

i.e. it is using my distribution-provided LLVM which is version 3.6. I wouldn't expect it to work with current version of Polly.

How do I specify which LLVM root to take? Setting `CMAKE_PREFIX_PATH` seems to work, but is it the intended way to do? Could you document it?

Next try by setting `CMAKE_PREFIX_PATH`:

CMake Error at cmake/polly_macros.cmake:23 (add_library):
add_library cannot create target "PollyISL" because an imported target with
the same name already exists.
Call Stack (most recent call first):
lib/External/CMakeLists.txt:262 (add_polly_library)

Polly is in my source checkout, so it got included automatically. I can switch it off using `-DLLVM_TOOL_POLLY_BUILD=OFF`. Of course it is not intended to build Polly externally if Polly is already available in LLVM, but it means I have to build some more LLVM builds (static/shared/dylib) around to test this.

With `-DLLVM_TOOL_POLLY_BUILD=OFF` it compiles, but almost all regression tests fail. For instance:

FAIL: Polly :: ScopInfo/wraping_signed_expr_1.ll (857 of 902)
******************** TEST 'Polly :: ScopInfo/wraping_signed_expr_1.ll' FAILED ********************
Script:
--
opt -load /root/build/polly/debug_findinstall/test/../lib/LLVMPolly.so -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-scops -analyze < /mnt/c/Users/Meinersbur/src/llvm/tools/polly/test/ScopInfo/wraping_signed_expr_1.ll | FileCheck /mnt/c/Users/Meinersbur/src/llvm/tools/polly/test/ScopInfo/wraping_signed_expr_1.ll
--
Exit Code: 2

Command Output (stderr):
--
#0 0x00000000013e4c6a (opt+0x13e4c6a)
#1 0x00000000013e29ee (opt+0x13e29ee)
#2 0x00000000013e2b62 (opt+0x13e2b62)
#3 0x00007f3d57bf1390 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
#4 0x00007f3d561d89f2 llvm::PointerIntPair<llvm::ilist_node_base<true>*, 1u, unsigned int, llvm::PointerLikeTypeTraits<llvm::ilist_node_base<true>*>, llvm::PointerIntPairInfo<llvm::ilist_node_base<true>*, 1u, llvm::PointerLikeTypeTraits<llvm::ilist_node_base<true>*> > >::getInt() const /root/install/llvm/release_nopolly/include/llvm/ADT/PointerIntPair.h:59:0
#5 0x00007f3d561d4312 llvm::ilist_node_base<true>::isSentinel() const /root/install/llvm/release_nopolly/include/llvm/ADT/ilist_node_base.h:46:0
#6 0x00007f3d561d4332 llvm::ilist_node_base<true>::isKnownSentinel() const /root/install/llvm/release_nopolly/include/llvm/ADT/ilist_node_base.h:47:0
#7 0x00007f3d561e234f llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::BasicBlock, true, false, void>, false, false>::operator*() const /root/install/llvm/release_nopolly/include/llvm/ADT/ilist_iterator.h:139:0
#8 0x00007f3d561d8cf5 llvm::simple_ilist<llvm::BasicBlock>::front() /root/install/llvm/release_nopolly/include/llvm/ADT/simple_ilist.h:134:0
#9 0x00007f3d561d4e44 llvm::Function::front() /root/install/llvm/release_nopolly/include/llvm/IR/Function.h:557:0
#10 0x00007f3d561d4e26 llvm::Function::getEntryBlock() /root/install/llvm/release_nopolly/include/llvm/IR/Function.h:534:0
#11 0x00007f3d561d0bb8 polly::ScopDetection::isValidRegion(polly::ScopDetection::DetectionContext&) const /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1510:0
#12 0x00007f3d561cf9d2 polly::ScopDetection::findScops(llvm::Region&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1330:0
#13 0x00007f3d561cfaf1 polly::ScopDetection::findScops(llvm::Region&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1341:0
#14 0x00007f3d561cab8a polly::ScopDetection::ScopDetection(llvm::Function&, llvm::DominatorTree const&, llvm::ScalarEvolution&, llvm::LoopInfo&, llvm::RegionInfo&, llvm::AAResults&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:295:0
#15 0x00007f3d561d1b10 polly::ScopDetectionWrapperPass::runOnFunction(llvm::Function&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1698:0
#16 0x0000000000ef5b33 (opt+0xef5b33)
#17 0x0000000000ef5bfc (opt+0xef5bfc)
#18 0x0000000000ef569d (opt+0xef569d)
#19 0x000000000066aa8b (opt+0x66aa8b)
#20 0x00007f3d56910830 __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:325:0
#21 0x00000000006a9559 (opt+0x6a9559)
Stack dump:
0. Program arguments: opt -load /root/build/polly/debug_findinstall/test/../lib/LLVMPolly.so -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-scops -analyze
1. Running pass 'Function Pass Manager' on module '<stdin>'.
2. Running pass 'Polly - Detect static control parts (SCoPs)' on function '@wrap'
FileCheck error: '-' is empty.
FileCheck command line: FileCheck /mnt/c/Users/Meinersbur/src/llvm/tools/polly/test/ScopInfo/wraping_signed_expr_1.ll

--

********************

There seem to be some link incompatibilities, but it works with in-LLVM-tree builds.



================
Comment at: CMakeLists.txt:27

+ else()
+ execute_process(COMMAND "${LLVM_INSTALL_ROOT}/bin/llvm-config" --src-root
+ OUTPUT_VARIABLE MAIN_SRC_DIR
----------------

`LLVM_INSTALL_ROOT` is nowhere defined.


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe added a comment.

In https://reviews.llvm.org/D33299#761863, @Meinersbur wrote:

> My first tryout failed with this error:

When importing a package you automatically get <PACK>_DIR variable pointing to the directory where the package Config is located. I.e., setting LLVM_DIR to ${LLVM_BUILD_TREE}/lib/cmake/llvm is what you want to do.

> Next try by setting `CMAKE_PREFIX_PATH`:

This works, too, however setting the LLVM_DIR variable instead usually is cleaner.

> Polly is in my source checkout, so it got included automatically. I can switch it off using `-DLLVM_TOOL_POLLY_BUILD=OFF`. Of course it is not intended to build Polly externally if Polly is already available in LLVM, but it means I have to build some more LLVM builds (static/shared/dylib) around to test this.

I understand. I'd expect switching LLVM_TOOL_POLLY_BUILD to OFF should work.

> With `-DLLVM_TOOL_POLLY_BUILD=OFF` it compiles, but almost all regression tests fail. For instance:

I can't reproduce this. I've seen similar errors before, though, when I broke ABI compatibilty between builds, e.g. by mixing Debug with Release, or mismatched SVN revisions.


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe updated this revision to Diff 100087.
philip.pfaffe added a comment.

- Fix location of llvm-config
- Don't copy update_check.py into llvm, as this will most certainly fail for an install tree.

https://reviews.llvm.org/D33299

Files:
CMakeLists.txt
lib/CMakeLists.txt

test/CMakeLists.txt

[Philip Pfaffe via Phabricator]

philip.pfaffe marked an inline comment as done.

https://reviews.llvm.org/D33299

[Michael Kruse via Phabricator]

Meinersbur added a comment.

In https://reviews.llvm.org/D33299#762008, @philip.pfaffe wrote:

> In https://reviews.llvm.org/D33299#761863, @Meinersbur wrote:
>
> > My first tryout failed with this error:
>
>
> When importing a package you automatically get <PACK>_DIR variable pointing to the directory where the package Config is located. I.e., setting LLVM_DIR to ${LLVM_BUILD_TREE}/lib/cmake/llvm is what you want to do.
>
> > Next try by setting `CMAKE_PREFIX_PATH`:
>
> This works, too, however setting the LLVM_DIR variable instead usually is cleaner.

Could you document this please?

>> With `-DLLVM_TOOL_POLLY_BUILD=OFF` it compiles, but almost all regression tests fail. For instance:
>
> I can't reproduce this. I've seen similar errors before, though, when I broke ABI compatibilty between builds, e.g. by mixing Debug with Release, or mismatched SVN revisions.

Is it possible to add some checks for whether LLVM and Polly are compatible?

In this case it was Release LLVM and Debug Polly. Because with Release-Release, I get the following error (for almost every test):

#0 0x00000000013e4c6a (opt+0x13e4c6a)
#1 0x00000000013e29ee (opt+0x13e29ee)
#2 0x00000000013e2b62 (opt+0x13e2b62)

#3 0x00007f63c93f1390 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
#4 0x00007f63c7b4c2e1 llvm::TerminatorInst::classof(llvm::Value const*) /root/src/llvm/include/llvm/IR/InstrTypes.h:80:0
#5 0x00007f63c7b4c2e1 llvm::isa_impl<llvm::TerminatorInst, llvm::User, void>::doit(llvm::User const&) /root/src/llvm/include/llvm/Support/Casting.h:57:0
#6 0x00007f63c7b4c2e1 llvm::isa_impl_cl<llvm::TerminatorInst, llvm::User const*>::doit(llvm::User const*) /root/src/llvm/include/llvm/Support/Casting.h:105:0
#7 0x00007f63c7b4c2e1 llvm::isa_impl_wrap<llvm::TerminatorInst, llvm::User const*, llvm::User const*>::doit(llvm::User const* const&) /root/src/llvm/include/llvm/Support/Casting.h:131:0
#8 0x00007f63c7b4c2e1 llvm::isa_impl_wrap<llvm::TerminatorInst, llvm::User* const, llvm::User const*>::doit(llvm::User* const&) /root/src/llvm/include/llvm/Support/Casting.h:123:0
#9 0x00007f63c7b4c2e1 bool llvm::isa<llvm::TerminatorInst, llvm::User*>(llvm::User* const&) /root/src/llvm/include/llvm/Support/Casting.h:142:0
#10 0x00007f63c7b4c2e1 llvm::PredIterator<llvm::BasicBlock, llvm::Value::user_iterator_impl<llvm::User> >::advancePastNonTerminators() /root/src/llvm/include/llvm/IR/CFG.h:47:0
#11 0x00007f63c7b4c2e1 llvm::PredIterator<llvm::BasicBlock, llvm::Value::user_iterator_impl<llvm::User> >::PredIterator(llvm::BasicBlock*) /root/src/llvm/include/llvm/IR/CFG.h:57:0
#12 0x00007f63c7b4c2e1 llvm::pred_begin(llvm::BasicBlock*) /root/src/llvm/include/llvm/IR/CFG.h:99:0
#13 0x00007f63c7b4c2e1 llvm::GraphTraits<llvm::Inverse<llvm::BasicBlock*> >::child_begin(llvm::BasicBlock*) /root/src/llvm/include/llvm/IR/CFG.h:191:0
#14 0x00007f63c7b4c2e1 llvm::iterator_range<llvm::GraphTraits<llvm::Inverse<llvm::BasicBlock*> >::ChildIteratorType> llvm::children<llvm::Inverse<llvm::BasicBlock*> >(llvm::GraphTraits<llvm::Inverse<llvm::BasicBlock*> >::NodeRef const&) /root/src/llvm/include/llvm/ADT/GraphTraits.h:108:0
#15 0x00007f63c7b4c2e1 llvm::LoopBase<llvm::BasicBlock, llvm::Loop>::getLoopLatches(llvm::SmallVectorImpl<llvm::BasicBlock*>&) const /root/src/llvm/include/llvm/Analysis/LoopInfo.h:246:0
#16 0x00007f63c7b4c2e1 polly::ScopDetection::canUseISLTripCount(llvm::Loop*, polly::ScopDetection::DetectionContext&) const /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1139:0
#17 0x00007f63c7b4c796 polly::ScopDetection::isValidLoop(llvm::Loop*, polly::ScopDetection::DetectionContext&) const /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1182:0
#18 0x00007f63c7b4e48d polly::ScopDetection::allBlocksValid(polly::ScopDetection::DetectionContext&) const /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1383:0
#19 0x00007f63c7b4e8e4 polly::ScopDetection::isValidRegion(polly::ScopDetection::DetectionContext&) const /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1515:0
#20 0x00007f63c7b4f526 polly::ScopDetection::findScops(llvm::Region&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1332:0
#21 0x00007f63c7b4f578 polly::ScopDetection::findScops(llvm::Region&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1341:0
#22 0x00007f63c7b4fa8b polly::ScopDetection::ScopDetection(llvm::Function&, llvm::DominatorTree const&, llvm::ScalarEvolution&, llvm::LoopInfo&, llvm::RegionInfo&, llvm::AAResults&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:295:0
#23 0x00007f63c7b5030e void std::swap<polly::ScopDetection*>(polly::ScopDetection*&, polly::ScopDetection*&) /usr/include/c++/5/bits/move.h:185:0
#24 0x00007f63c7b5030e std::unique_ptr<polly::ScopDetection, std::default_delete<polly::ScopDetection> >::reset(polly::ScopDetection*) /usr/include/c++/5/bits/unique_ptr.h:342:0
#25 0x00007f63c7b5030e polly::ScopDetectionWrapperPass::runOnFunction(llvm::Function&) /root/src/llvm/tools/polly/lib/Analysis/ScopDetection.cpp:1698:0
#26 0x0000000000ef5b33 (opt+0xef5b33)
#27 0x0000000000ef5bfc (opt+0xef5bfc)
#28 0x0000000000ef569d (opt+0xef569d)
#29 0x000000000066aa8b (opt+0x66aa8b)
#30 0x00007f63c8110830 __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:325:0
#31 0x00000000006a9559 (opt+0x6a9559)

(I switched to debug to get symbols; to get this trace I manually added "-g" to CMAKE_CXX_FLAGS)

The revisions are compatible, the in-LLVM-tree builds work well. It also works with the previous llvm-config based trunk (even mixes of Release/Debug build). Any other idea?


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe added a comment.

In https://reviews.llvm.org/D33299#763275, @Meinersbur wrote:

> In https://reviews.llvm.org/D33299#762008, @philip.pfaffe wrote:
>
> >
>
>
> Is it possible to add some checks for whether LLVM and Polly are compatible?

There's the compile-time check for EnableABIBreakingChecks, but I don't think that'll help in this case.

> In this case it was Release LLVM and Debug Polly. Because with Release-Release, I get the following error (for almost every test):

> [...]

> The revisions are compatible, the in-LLVM-tree builds work well. It also works with the previous llvm-config based trunk (even mixes of Release/Debug build). Any other idea?

I have the strong suspicion that this is still a version incompatibility. My patch is missing bigger changes to test/CMakeLists.txt, which still uses LLVM_INSTALL_ROOT. Hence, I think your test-suite is picking up your distribution version of opt. I'll drop the distinction there.

At this point, it might be worthwhile to consider getting rid of the 'copy FileCheck to TOOLS_BINARY_DIR' requirement. CMake has a nice feature for that, which is `find_program()`. It autodetects programs and further makes this configurable. Plugging this into the lit test-suite is however not super beautiful. Options I see are:

1. From the found program, determine the PATH of the program and hand it into the lit config. This might create potentially confusing results, if tools by the same name are in multiple paths, and one in a path not specified by the user wins.
2. Pass the actual found programs into the lit config using substitutions. That would mean in all existing testcases replace (e.g.), opt with %opt.

I'm slightly in favor of the first option, because we don't have to adapt //all// the testcases, Thoughts?


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe updated this revision to Diff 100394.
philip.pfaffe added a comment.

Fixed the lit tests CMakeLists.

I've implemented option 1) i mentioned above. This now comes with the gotcha I mentioned, that even though you specify a tool it might may use a different one instead, if it also exists in the other tools' paths. Because that's most troublesome for opt, I've put the opt path first to give it precedence.


https://reviews.llvm.org/D33299

Files:
CMakeLists.txt
cmake/CMakeLists.txt
lib/CMakeLists.txt
test/CMakeLists.txt
test/lit.cfg
test/lit.site.cfg.in

[Tom Stellard via Phabricator]

tstellar added a comment.

I think this is a good idea. I submitted similar patches for clang:

https://reviews.llvm.org/D32577
https://reviews.llvm.org/D32578
https://reviews.llvm.org/D32595
https://reviews.llvm.org/D32604


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe added a comment.

In https://reviews.llvm.org/D33299#761863, @Meinersbur wrote:

> How do I specify which LLVM root to take? Setting `CMAKE_PREFIX_PATH` seems to work, but is it the intended way to do?

I'd like to amend my previous response to this: Setting `CMAKE_PREFIX_PATH` is in fact the more //general// way to do this. While setting `package_DIR` is guaranteed to work universally for `find_package(CONFIG)`, it is not mandatory for find-modules (in current versions of CMake).


https://reviews.llvm.org/D33299

[Michael Kruse via Phabricator]

Meinersbur added a comment.

This works for me with and without `-DBUILD_SHARED=ON`. However, with LLVM configured `-DLLVM_BUILD_LLVM_DYLIB=ON`, `-DLLVM_LINK_LLVM_DYLIB=ON` things don't work as smoothly.

When Polly is configured to LLVM's build directory, `LLVM_LINK_LLVM_DYLIB` is ignored. OK, I can live with that.

When Polly is configured to LLVM's install directory (`CMAKE_INSTALL_PREFIX`), I get errors:

llvm-lit: /root/src/llvm/utils/lit/lit/formats/googletest.py:41: warning: unable to discover google-tests in '/root/build/polly/release_findbuild_dylib/unittests/DeLICM/DeLICMTests': Command '['/root/build/polly/release_findbuild_dylib/unittests/DeLICM/DeLICMTests', '--gtest_list_tests']' returned non-zero exit status 1. Process output:
: CommandLine Error: Option 'debug' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

llvm-lit: /root/src/llvm/utils/lit/lit/formats/googletest.py:41: warning: unable to discover google-tests in '/root/build/polly/release_findbuild_dylib/unittests/Flatten/FlattenTests': Command '['/root/build/polly/release_findbuild_dylib/unittests/Flatten/FlattenTests', '--gtest_list_tests']' returned non-zero exit status 1. Process output:
: CommandLine Error: Option 'debug' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

llvm-lit: /root/src/llvm/utils/lit/lit/formats/googletest.py:41: warning: unable to discover google-tests in '/root/build/polly/release_findbuild_dylib/unittests/Isl/IslTests': Command '['/root/build/polly/release_findbuild_dylib/unittests/Isl/IslTests', '--gtest_list_tests']' returned non-zero exit status 1. Process output:
: CommandLine Error: Option 'debug' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

llvm-lit: /root/src/llvm/utils/lit/lit/formats/googletest.py:41: warning: unable to discover google-tests in '/root/build/polly/release_findbuild_dylib/unittests/ScopPassManager/ScopPassManagerTests': Command '['/root/build/polly/release_findbuild_dylib/unittests/ScopPassManager/ScopPassManagerTests', '--gtest_list_tests']' returned non-zero exit status 1. Process output:
: CommandLine Error: Option 'debug' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

llvm-lit: /root/src/llvm/utils/lit/lit/discovery.py:190: warning: test suite 'Polly-Unit' contained no tests

The linking command reveals that, in addition to `libLLVM-5.0svn.so`, it also links to all of *.a from the build directory:

/usr/bin/c++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -w -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings -fno-exceptions -fno-rtti -O3 -Wl,-O3 -Wl,--gc-sections unittests/DeLICM/CMakeFiles/DeLICMTests.dir/DeLICMTest.cpp.o -o unittests/DeLICM/DeLICMTests /root/build/llvm/release_nopolly_dylib/lib/libLLVMSupport.a -lpthread /root/build/llvm/release_nopolly_dylib/lib/libgtest_main.a /root/build/llvm/release_nopolly_dylib/lib/libgtest.a -lpthread lib/libPolly.a -lpthread /root/build/llvm/release_nopolly_dylib/lib/libLLVM-5.0svn.so lib/External/libPollyPPCG.a lib/External/libPollyISL.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMPasses.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMNVPTXCodeGen.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMipo.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMIRReader.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMAsmParser.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMInstrumentation.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMLinker.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMAsmPrinter.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMDebugInfoCodeView.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMDebugInfoMSF.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMSelectionDAG.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMCodeGen.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMScalarOpts.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMInstCombine.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMBitWriter.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMNVPTXDesc.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMNVPTXAsmPrinter.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMNVPTXInfo.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMTarget.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMVectorize.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMTransformUtils.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMAnalysis.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMObject.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMBitReader.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMMCParser.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMMC.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMProfileData.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMCore.a /root/build/llvm/release_nopolly_dylib/lib/libLLVMSupport.a -lrt -ldl -ltinfo -lpthread -lz -lm /root/build/llvm/release_nopolly_dylib/lib/libLLVMDemangle.a -Wl,-rpath,/root/build/llvm/release_nopolly_dylib/lib

If you don't want to support `LLVM_LINK_LLVM_DYLIB` configurations, I suggest adding an error message notifying the user about it.

When I remove the llvm source directory and configure Polly to use LLVM's install path (Once installed, it should work without the source directory, right?), I get this error:

[140/140] Running polly regression tests
Traceback (most recent call last):
File "/root/install/llvm/release_nopolly/bin/llvm-lit", line 53, in <module>
from lit.main import main
ImportError: No module named lit.main
FAILED: cd /root/build/polly/release_findinstall/test && /root/install/llvm/release_nopolly/bin/llvm-lit --param polly_site_config=/root/build/polly/release_findinstall/test/lit.site.cfg --param polly_unit_site_config=/root/build/polly/release_findinstall/test/Unit/lit.site.cfg /root/build/polly/release_findinstall/test
ninja: build stopped: subcommand failed.

I am not sure this even worked before. Why is an installed llvm-lit importing something from the source directory? Is it by design? Does this mean one cannot run `check-polly` from a distribution-supplied LLVM?


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe added a comment.

I'm unable to reproduce your issue with DYLIB. Are there any special options you're setting on the Polly side? As far as i can see, the LLVM_*_LLVM_DYLIB options aren't exported. How is Polly picking up libLLVM for you?

Regarding your second issue: llvm-lit is not purposed for distribution. You can't really use it out of the llvm source tree. I'm guessing you've copied that to the install tree at some point manually (because it's what the current Polly setup requires you to do). The good news is, with my `find_program` change above, you can just use any (proper) installation of lit, e.g. through pip. The bad news is, this still doesn't work if the llvm source tree is missing because of a bug in the unittest lit config. I'll follow up on that.


https://reviews.llvm.org/D33299

[Michael Kruse via Phabricator]

Meinersbur added a comment.

> How is Polly picking up libLLVM for you?

LLVM (script located at ~/build/llvm/release_nopolly_dylib.sh):

#! /usr/bin/env bash
set -e

TYPENAME=`basename $0 .sh`
BUILDPATH=`realpath --no-symlinks $(dirname $0)`
PROJECTNAME=`basename ${BUILDPATH}`
TYPEPATH="${BUILDPATH}/${TYPENAME}"
SRCPATH=~/src/${PROJECTNAME}

mkdir -p "${TYPEPATH}"
cd "${TYPEPATH}"

cmake "${SRCPATH}" -GNinja \
-DLLVM_BUILD_LLVM_DYLIB=ON \
-DLLVM_LINK_LLVM_DYLIB=ON \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX=${HOME}/install/${PROJECTNAME}/${TYPENAME} \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DLLVM_PARALLEL_LINK_JOBS=1 \
-DLLVM_TARGETS_TO_BUILD=X86\;NVPTX \
-DLLVM_ENABLE_ASSERTIONS=ON \
-DLIBCXX_ENABLE_ASSERTIONS=ON \
-DLIBCXXABI_ENABLE_ASSERTIONS=ON \
-DLLVM_TOOL_POLLY_BUILD=OFF \
-DLLVM_POLLY_BUILD=OFF \
-DLLVM_TOOL_LLDB_BUILD=OFF \
-DLLVM_LLDB_BUILD=OFF \
-DLLVM_TOOL_LLD_BUILD=OFF \
-DLLVM_LLD_BUILD=OFF \
-DLLVM_TOOL_OPENMP_BUILD=OFF \
-DLLVM_OPENMP_BUILD=OFF \
-DLLVM_TOOL_COMPILER_RT_BUILD=OFF \
-DLLVM_COMPILER_RT_BUILD=OFF \
-DCLANG_TOOL_EXTRA_BUILD=OFF \
-DCLANG_EXTRA_BUILD=OFF \
-DLLVM_ENABLE_LIBCXX=OFF \
-DLLVM_TOOL_LIBCXX_BUILD=OFF \
-DLLVM_LIBCXX_BUILD=OFF \
-DLLVM_TOOL_LIBCXXABI_BUILD=OFF \
-DLLVM_LIBCXXABI_BUILD=OFF \

Polly:

#! /usr/bin/env bash
set -e

TYPENAME=`basename $0 .sh`
BUILDPATH=`realpath --no-symlinks $(dirname $0)`
PROJECTNAME=`basename ${BUILDPATH}`
TYPEPATH="${BUILDPATH}/${TYPENAME}"
SRCPATH=~/src/llvm/tools/${PROJECTNAME}

mkdir -p "${TYPEPATH}"
cd "${TYPEPATH}"

cmake "${SRCPATH}" -GNinja \
-DLLVM_INSTALL_ROOT=${HOME}/build/llvm/release_nopolly_dylib \
-DCMAKE_PREFIX_PATH=${HOME}/build/llvm/release_nopolly_dylib \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX=${HOME}/install/${PROJECTNAME}/${TYPENAME} \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DLLVM_ENABLE_ASSERTIONS=ON \
-DPOLLY_ENABLE_GPGPU_CODEGEN=ON

This automatically finds `FileCheck` in the build dir, which is nice, but fails with the aforementioned error.

`-DLLVM_DIR=-DLLVM_DIR=${HOME}/build/llvm/release_nopolly_dylib` doesn't work.


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe added a comment.

Your script works out of the box for me. Did you configure this in a clean environment?

Your first snippet ends in a `\`, is it missing any options?

> `-DLLVM_DIR=-DLLVM_DIR=${HOME}/build/llvm/release_nopolly_dylib` doesn't work.

There's one -D too many here (typo?), and the package path should be `${HOME}/build/llvm/release_nopolly_dylib/lib/cmake/llvm`.


https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

I will mark this LGTM, as the general idea is OK. If you both can come to an agreement, feel free to go ahead. (or add me back if you need more feedback)


https://reviews.llvm.org/D33299

[Philip Pfaffe via Phabricator]

philip.pfaffe added a comment.

@Meinersbur Ping?


https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser added a comment.

@Meinersbur : Hi Michael, do you have any comments on this (would you like to review this as well?) or can this be committed.


https://reviews.llvm.org/D33299

[Tobias Grosser via Phabricator]

grosser added a comment.

Hi Philip,

Michael is still on vacation. I suggest to commit this change. Michael can do a post-commit review if needed.


https://reviews.llvm.org/D33299

[Phabricator via Phabricator]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL307650: [Polly][CMake] Use the CMake Package instead of llvm-config in out-of-tree… (authored by pfaffe).

Changed prior to commit:
https://reviews.llvm.org/D33299?vs=100394&id=106001#toc

Repository:
rL LLVM

https://reviews.llvm.org/D33299

Files:
polly/trunk/CMakeLists.txt
polly/trunk/cmake/CMakeLists.txt
polly/trunk/lib/CMakeLists.txt
polly/trunk/test/CMakeLists.txt
polly/trunk/test/lit.cfg
polly/trunk/test/lit.site.cfg.in