[polly] static builds

Tobias Grosser tobias at grosser.es
Sat Mar 8 12:05:33 PST 2014


On 03/07/2014 06:29 PM, Sebastian Pop wrote:
> Hi Tobi,
>
> I just updated the two patches against llvm and polly to be able to link Polly
> statically in all LLVM tools.  I think I addressed all your comments below...
> except that the static initializers of Polly do not seem to work, so even if now
> we can link Polly statically, all the passes Polly provides are not registered,
> so make check-polly does not pass.

Hey Sebastian,

those patches are amazingly small, but unfortunately there are more 
problems than just static initializers that are not run.

> Can we work together incrementally from these patches to address the missing
> bits in initializing Polly passes?

We very much should work together on this.

Are you also asking if we can commit those changes before having 
addressed the open problem? In case you do, I don't think that's the 
right way. We can always commit functional parts, but not something that 
does not work yet.

>  From 3a17cb49dc9286a7ac3c1c1cd94cb14323c5790f Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Thu, 31 Oct 2013 06:38:45 -0500
> Subject: [PATCH] link polly statically
>
> ---
>   CMakeLists.txt                    |    8 ++++++++
>   lib/Transforms/IPO/CMakeLists.txt |    4 ++++
>   tools/CMakeLists.txt              |    9 ++++++++-
>   3 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index f3a09dd..2699b39 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -562,3 +562,11 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
>       )
>   endif()
>
> +option(WITH_POLLY "Build LLVM with Polly" ON)

The WITH_POLLY option seems an independent fix for llvm.org/PR13550, and 
should probably also be used to guard the later 
add_llvm_external_project() call.

Such an patch is preapproved to commit immediately.

> +option(POLLY_BUILD_SHARED_LIB "Build Polly as a shared library" ON)

I do not see any harm in building the shared library always. Also, 
getting the connection that disabling the shared library build causes 
Polly to directly be linked into clang is non-obvious. What about 
calling this option POLLY_LINK_POLLY_INTO_TOOLS=OFF and use it to enable 
the linking into the various tools.

(This comment is not obligatory. We can still change this in a 
subsequent patch or after making this patch actually work)

> +if(WITH_POLLY)
> +  if(NOT EXISTS ${LLVM_MAIN_SRC_DIR}/tools/polly/CMakeLists.txt)
> +    set(WITH_POLLY OFF)
> +  endif()
> +endif(WITH_POLLY)

This is part of the first patch, that I preapproved above.

> diff --git a/lib/Transforms/IPO/CMakeLists.txt b/lib/Transforms/IPO/CMakeLists.txt
> index 90c1c33..6460774 100644
> --- a/lib/Transforms/IPO/CMakeLists.txt
> +++ b/lib/Transforms/IPO/CMakeLists.txt
> @@ -23,3 +23,7 @@ add_llvm_library(LLVMipo
>     )
>
>   add_dependencies(LLVMipo intrinsics_gen)
> +
> +if (WITH_POLLY AND NOT POLLY_BUILD_SHARED_LIB)
> +  target_link_libraries(LLVMipo LLVMPolly)
> +endif (WITH_POLLY AND NOT POLLY_BUILD_SHARED_LIB)

This looks like a little hack. The cleaner solution would be to link 
polly directly into opt, bugpoint and clang, no?

I also spend a couple of hours to investigate the static initializer
issue. The problem is that the binaries are not using any functionality 
from within Polly. If there is no polly functionality used, the polly 
library will not be linked in at all. This means the static initializers 
are also not linked in and are consequently not run either. I did not 
expect this, but it seems we need to call at least some functionality 
from polly e.g polly::initializePollyPasses(Registry). After this, the 
static initializers should also be included and hopefully run.

I briefly checked if this is sufficient, but it seems we also need to 
link against isl. We do not see an error yet, as all functionality that 
uses isl is currently not even linked in, but as soon as the above fix 
is done, we get errors about missing references to isl functions. It 
seems this functionality was already part of the previous patch but you 
seemed to have removed it again.

> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index 6223d1ca..a091e3b 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
> @@ -1,5 +1,10 @@
>   add_llvm_tool_subdirectory(llvm-config)
>
> +# Build polly before opt, as opt links against polly.
> +if(NOT POLLY_BUILD_SHARED_LIB)
> +  add_llvm_external_project(polly)
> +endif()
> +
>   add_llvm_tool_subdirectory(opt)
>   add_llvm_tool_subdirectory(llvm-as)
>   add_llvm_tool_subdirectory(llvm-dis)
> @@ -66,7 +71,9 @@ add_llvm_external_project(clang)
>   if( NOT LLVM_INCLUDE_TOOLS STREQUAL "bootstrap-only" )
>     add_llvm_external_project(lld)
>     add_llvm_external_project(lldb)
> -  add_llvm_external_project(polly)
> +  if(POLLY_BUILD_SHARED_LIB)
> +    add_llvm_external_project(polly)
> +  endif(POLLY_BUILD_SHARED_LIB)

I do not see why we would make this conditionally. I rather would just 
move this always further up.

This change could also be part of the WITH_POLLY patch.

>     # Automatically add remaining sub-directories containing a 'CMakeLists.txt'
>     # file as external projects.
> -- 1.7.6.4
>
>
> 0001-static-link-polly.patch
>
>
>  From 06a118041c279b328dbada9e37a2bf6ea6aced76 Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Thu, 6 Mar 2014 16:32:13 -0600
> Subject: [PATCH] static link polly
>
> ---
>   lib/CMakeLists.txt   |   40 ++++++++++++++++++++++++++--------------
>   test/lit.site.cfg.in |   15 +++++++++++++--
>   2 files changed, 39 insertions(+), 16 deletions(-)
>
> diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
> index dc66491..0df059d 100644
> --- a/lib/CMakeLists.txt
> +++ b/lib/CMakeLists.txt
> @@ -1,6 +1,3 @@
> -# build a monolithic libLLVMPollyLib.$shlibext
> -# and a thin module LLVMPolly.moduleext that links to that shared library
> -
>   set(LLVM_NO_RTTI 1)
>
>   if (PLUTO_FOUND)
> @@ -42,7 +39,7 @@ if (SCOPLIB_FOUND)
>         Exchange/ScopLibImporter.cpp)
>   endif (SCOPLIB_FOUND)
>
> -add_polly_library(LLVMPollyLib
> +set(POLLY_FILES
>     Analysis/Dependences.cpp
>     Analysis/ScopDetection.cpp
>     Analysis/ScopInfo.cpp
> @@ -74,22 +71,37 @@ add_polly_library(LLVMPollyLib
>     ${POLLY_PLUTO_FILES}
>     )
>
> -add_polly_loadable_module(LLVMPolly
> -  PollyModule.cpp
> -)
> +if (POLLY_BUILD_SHARED_LIB)
> +  # build a monolithic libLLVMPollyLib.$shlibext
> +  # and a thin module LLVMPolly.moduleext that links to that shared library
> +  add_polly_library(LLVMPollyLib ${POLLY_FILES})
> +  add_polly_loadable_module(LLVMPolly PollyModule.cpp)
> +  target_link_libraries(LLVMPolly LLVMPollyLib)
> +  set_target_properties(LLVMPolly
> +    PROPERTIES
> +    LINKER_LANGUAGE CXX
> +    PREFIX "")
> +else (POLLY_BUILD_SHARED_LIB)
> +  set(LLVM_OPTIONAL_SOURCES
> +    CodeGen/Cloog.cpp
> +    CodeGen/CodeGeneration.cpp
> +    CodeGen/PTXGenerator.cpp
> +    Exchange/OpenScopImporter.cpp
> +    Exchange/OpenScopExporter.cpp
> +    Exchange/ScopLib.cpp
> +    Exchange/ScopLibExporter.cpp
> +    Exchange/ScopLibImporter.cpp
> +    Pluto.cpp
> +    Pocc.cpp
> +    PollyModule.cpp)
 >
> +  add_llvm_library(LLVMPolly ${POLLY_FILES} PollyModule.cpp)
> +endif (POLLY_BUILD_SHARED_LIB)

This seems to duplicate a lot of logic. Maybe we can just link 
libLLVMPollylib into opt and Co?

(We can work on this later after this patch actually works)

>   if (TARGET intrinsics_gen)
>     # Check if we are building as part of an LLVM build
>     add_dependencies(LLVMPolly intrinsics_gen)
>   endif()
>
> -target_link_libraries(LLVMPolly LLVMPollyLib)
> -
> -set_target_properties(LLVMPolly
> -  PROPERTIES
> -  LINKER_LANGUAGE CXX
> -  PREFIX "")
> -
>   if (PLUTO_FOUND)
>     target_link_libraries(LLVMPolly ${PLUTO_LIBRARY})
>   endif(PLUTO_FOUND)
> diff --git a/test/lit.site.cfg.in b/test/lit.site.cfg.in
> index d87f2fe..2bd39c6 100644
> --- a/test/lit.site.cfg.in
> +++ b/test/lit.site.cfg.in
> @@ -25,8 +25,19 @@ except KeyError,e:
>       key, = e.args
>       lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
>
> -config.substitutions.append(('%loadPolly', '-load '
> -                             + config.polly_lib_dir + '/LLVMPolly at LLVM_SHLIBEXT@'))
> +if '@POLLY_BUILD_SHARED_LIB@' == '' or \

Can you assign those names at the top to a config.* variable?

> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == '0' or \
> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == 'n' or \
> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == 'no' or \
> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == 'off' or \
> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == 'false' or \
> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == 'notfound' or \
> +   '@POLLY_BUILD_SHARED_LIB@'.lower() == 'polly_build_shared_lib-notfound':

I am a little bit surprised, there are so many spellings. We can 
obviously keep them, but could you give me a short explanation why this 
can not only be 'ON' and 'OFF'?

(We can work on this later after this patch actually works)

Tobias




More information about the llvm-commits mailing list