[PATCH] D32442: [Polly][CMake] Use object library to build two flavours of Polly.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/CMakeLists.txt:75
+# well.
+add_polly_library(Polly $<TARGET_OBJECTS:PollyCore>)
+target_link_libraries(Polly
----------------
singam-sanjay wrote:
> Why can't we just add PollyCore as a dependency ?
That should work as well, if PollyCore was a static library. For OBJECT libraries, this is the only way to do. See https://cmake.org/cmake/help/v3.8/command/add_library.html#object-libraries

Here are the reasons:
- LLVM also creates OBJECT libraries when it creates static and shared libraries in one build.
- There is one less library that could be either a shared or static library.
- Without it, the Polly and LLVM library would be empty. This could cause symbols of PollyCore to not get pulled-in by the linker, which gets us into trouble.
- CMake might order PollyCore behind PollyISL and PollyPPCG in the linker command linker, meaning it would not pull anything from them because theirs symbols were not required then.


================
Comment at: lib/CMakeLists.txt:131
+    else ()
+      target_link_libraries(Polly
+        LLVMNVPTXCodeGen
----------------
singam-sanjay wrote:
> We need **all** these libraries and not just those that might be returned by `llvm_map_components_to_libnames(nvptx_libs NVPTX)` (I'm assuming that you mean nvptx_libs might turn out to be a subset of all possible NVPTX libraries).
> 
> Nevertheless, won't all LLVMNVPTX* libraries be included in nvptx_libs since they are **all** a part of the NVPTX backend ?
There are two separate cases:

* We are building inside the LLVM tree:
  Then cmake knowns which libraries NVPTX* depend on and adjust the linker command line accordingly.
   This is exactly how LLVM's CMakeLists.txt do this (using LLVM_LINK_COMPONENTS)

* We are building out-of-tree:
  Then we don't know the NVPTX* dependencies. however, we link against all the libraries (using LLVM_LIBS), so all its dependencies are already available (including NVPTX*, so the second case is kind-of redundant, I might remove them).

I think I don't actually understood your question. What are "all" libraries? `llvm_map_components_to_libnames` returns: LLVMNVPTXCodeGen LLVMNVPTXInfo LLVMNVPTXDesc LLVMNVPTXAsmPrinter.


================
Comment at: unittests/CMakeLists.txt:12
-
-    # The Polly target does not depend on its required libraries, except:
-    # - BUILD_SHARED_LIBS=ON
----------------
singam-sanjay wrote:
> `patch` failed to update this file because it couldn't find these comments. At which commit did you create the diff ?
The comments were added in r301095 ([CMake] Fix unittests in out-of-LLVM-tree builds.)


https://reviews.llvm.org/D32442