[polly] static builds
Tobias Grosser
tobias at grosser.es
Thu Nov 21 04:20:36 PST 2013
On 11/15/2013 10:21 PM, Tobias Grosser wrote:
> On 11/15/2013 09:48 PM, Sebastian Pop wrote:
>> To ease review, please find the patches attached.
>
> Hi Sebastian,
>
> sorry for the delay.
>
> As an overall comment, I think the intention of the patch is good and
> this also seems to be in line of the feedback we got on the developer
> meeting. Thanks a lot for working on this!
I had another look. I have seen that you committed the first pieces of
this patch already. This is great!
Here my previous comments, with some additional feedback.
>>>> > >diff --git a/CMakeLists.txt b/CMakeLists.txt
>>>> > >index 096e3e7..895077c 100644
>>>> > >--- a/CMakeLists.txt
>>>> > >+++ b/CMakeLists.txt
>>>> > >@@ -527,3 +527,17 @@ if(MSVC_VERSION EQUAL 1600)
>>>> > > file(APPEND "${LLVM_SLN_FILENAME}" "\n# This should be
>>>> regenerated!\n")
>>>> > > endif()
>>>> > > endif()
>>>> > >+
>>>> > >+option(WITH_POLLY "Build LLVM with Polly" OFF)
>
> Why is this default to OFF? Can we make this default to on and in
> addition check if polly directory exists. If people have Polly checked
> out, I believe it is very likely that they would like to build it.
Still applies.
>>>> > >+option(POLLY_BUILD_SHARED_LIB "Build Polly as a shared library
>>>> (only static builds are supported under Windows)" ON)
>
>>>> > >+if(NOT WITH_POLLY)
>>>> > >+ if(LLVM_EXTERNAL_POLLY_BUILD OR NOT
>>>> LLVM_EXTERNAL_POLLY_SOURCE_DIR STREQUAL "")
>>>> > >+ set(WITH_POLLY ON)
>>>> > >+ endif()
>
> The two EXTERNAL* variables, are they needed in the first patch?
> Otherwise, I propose to first get the basic functionality in then add
> patches later if needed.
This still applies. It would be nice to see a new patch, without the
already committed pieces and if possible, without this additional
functionality.
>>>> > >+endif(NOT WITH_POLLY)
>>>> > >+
>>>> > >+if(WITH_POLLY)
>>>> > >+ FIND_PACKAGE(Isl REQUIRED)
>>>> > >+ FIND_PACKAGE(Gmp REQUIRED)
>
> Is GMP really required? It seems if we build without CLooG there should
> be no need for it, no? (isl may require it, but there is no direct
> dependency, no?) Or is this to statically link gmp and isl?
>
> Also, I do not see a requirement for CLooG. Does this mean Polly will be
> built without CLooG if compiled into LLVM?
Still applies.
>>>> > >+endif(WITH_POLLY)
>
>>>> > >diff --git a/cmake/modules/FindGmp.cmake
>>>> b/cmake/modules/FindGmp.cmake
>>>> > >new file mode 100644
>>>> > >index 0000000..3725d42
>>>> > >--- /dev/null
>>>> > >+++ b/cmake/modules/FindGmp.cmake
>>>> > >@@ -0,0 +1,21 @@
>>>> > >+FIND_PATH(GMP_INCLUDE_DIR gmp.h)
>>>> > >+
>>>> > >+FIND_LIBRARY(GMP_LIBRARY NAMES gmp)
>>>> > >+
>>>> > >+IF (POLLY_USE_GMP)
>>>> > >+ IF (GMP_INCLUDE_DIR AND GMP_LIBRARY)
>>>> > >+ SET(GMP_FOUND TRUE)
>>>> > >+ ENDIF (GMP_INCLUDE_DIR AND GMP_LIBRARY)
>>>> > >+ENDIF (POLLY_USE_GMP)
Do we need to check POLLY_USE_GMP here? I would probably check this
outside of FindGmp, similar to the recently committed POLLY_USE_CLOOG.
Also, this seems to be independent of statically building Polly. Maybe
we can addres
>>>> > >+IF (GMP_FOUND)
>>>> > >+ IF (NOT GMP_FIND_QUIETLY)
>>>> > >+ MESSAGE(STATUS "Found GMP: ${GMP_LIBRARY}")
>>>> > >+ ENDIF (NOT GMP_FIND_QUIETLY)
>>>> > >+ELSE (GMP_FOUND)
>>>> > >+ IF (GMP_FIND_REQUIRED)
>>>> > >+ MESSAGE(FATAL_ERROR "Could not find GMP")
>>>> > >+ ENDIF (GMP_FIND_REQUIRED)
>>>> > >+ENDIF (GMP_FOUND)
>
> I wonder, can we not reuse the ones already available in Polly, no?
Still applies.
>>>> > >diff --git a/cmake/modules/FindIsl.cmake
>>>> b/cmake/modules/FindIsl.cmake
>>>> > >new file mode 100644
>>>> > >index 0000000..bf7a0a0
>>>> > >--- /dev/null
>>>> > >+++ b/cmake/modules/FindIsl.cmake
>>>> > >@@ -0,0 +1,27 @@
>>>> > >+IF (POLLY_EXTERNAL_ISL_SOURCE_DIR)
>>>> > >+ add_subdirectory(${POLLY_EXTERNAL_ISL_SOURCE_DIR}
>>>> "${CMAKE_CURRENT_BINARY_DIR}/isl")
>>>> > >+ IF (POLLY_BUILD_SHARED_LIB)
>>>> > >+ SET(ISL_LIBRARY "${ISL_BINARY_DIR}/lib/isl.so")
>>>> > >+ ELSE(POLLY_BUILD_SHARED_LIB)
>>>> > >+ SET(ISL_LIBRARY "${ISL_BINARY_DIR}/lib/isl.a")
>>>> > >+ ENDIF(POLLY_BUILD_SHARED_LIB)
>
> I wonder what the best practice here is. I would assume that FindIsl
> should just detect the locations of both the isl.so and the isl.a file
> and at the location they are used, the correct library should then be
> choosen.
Still applies.
>>>> > >+ SET(ISL_INCLUDE_DIR "${POLLY_EXTERNAL_ISL_SOURCE_DIR}/include")
>>>> > >+ INCLUDE_DIRECTORIES("${POLLY_EXTERNAL_ISL_SOURCE_DIR}/imath")
>
> imath? Why is this needed? With the new isl interface, the math library
> in isl should be completely hidden.
Still applies.
>>>> > >+ENDIF (POLLY_EXTERNAL_ISL_SOURCE_DIR)
>
> The whole POLLY_EXTERNAL_ISL_SOURCE_DIR seems to be an additional
> feature. Maybe we can leave it out of the first code review.
Still applies.
>>>> > >+
>>>> > >+FIND_PATH(ISL_INCLUDE_DIR isl/val.h)
>>>> > >+FIND_LIBRARY(ISL_LIBRARY NAMES isl)
>>>> > >+
>>>> > >+IF (ISL_INCLUDE_DIR AND ISL_LIBRARY)
>>>> > >+ SET(ISL_FOUND TRUE)
>>>> > >+ENDIF (ISL_INCLUDE_DIR AND ISL_LIBRARY)
>>>> > >+
>>>> > >+IF (ISL_FOUND)
>>>> > >+ IF (NOT Isl_FIND_QUIETLY)
>>>> > >+ MESSAGE(STATUS "Found Isl: ${ISL_LIBRARY}")
>>>> > >+ ENDIF (NOT Isl_FIND_QUIETLY)
>>>> > >+ELSE (ISL_FOUND)
>>>> > >+ IF (Isl_FIND_REQUIRED)
>>>> > >+ MESSAGE(FATAL_ERROR "Could not find Isl")
>>>> > >+ ENDIF (Isl_FIND_REQUIRED)
>>>> > >+ENDIF (ISL_FOUND)
>
> The same applies here, can we use the one in Polly?
I meant, can we use the FindIsl file available in tools/polly/cmake to
avoid code duplication?
>>>> > >diff --git a/lib/Transforms/IPO/PassManagerBuilder.cpp
>>>> b/lib/Transforms/IPO/PassManagerBuilder.cpp
>>>> > >index 0017c1b..083055f 100644
>>>> > >--- a/lib/Transforms/IPO/PassManagerBuilder.cpp
>>>> > >+++ b/lib/Transforms/IPO/PassManagerBuilder.cpp
>>>> > >@@ -105,8 +105,18 @@
>>>> PassManagerBuilder::addInitialAliasAnalysisPasses(PassManagerBase
>>>> &PM) const {
>>>> > > PM.add(createBasicAliasAnalysisPass());
>>>> > > }
>>>> > >
>>>> > >+#if defined(WITH_POLLY) && !defined(POLLY_BUILD_SHARED_LIB)
>>>> > >+void registerPollyEarlyAsPossiblePasses(const
>>>> llvm::PassManagerBuilder &Builder,
>>>> > >+ llvm::PassManagerBase &PM);
>>>> > >+void registerPollyOptLevel0Passes(const llvm::PassManagerBuilder
>>>> &Builder,
>>>> > >+ llvm::PassManagerBase &PM);
>>>> > >+#endif // WITH_POLLY
>
> I think including a header with those definitions would be better style.
> However, my preferred solution would be to use the static initializer in
> Polly.
Still applies.
>>>> > > void
>>>> PassManagerBuilder::populateFunctionPassManager(FunctionPassManager
>>>> &FPM) {
>>>> > > addExtensionsToPM(EP_EarlyAsPossible, FPM);
>>>> > >+#if defined(WITH_POLLY) && !defined(POLLY_BUILD_SHARED_LIB)
>>>> > >+ registerPollyEarlyAsPossiblePasses(*this, FPM);
>>>> > >+#endif // WITH_POLLY
>>>> > >
>>>> > > // Add LibraryInfo if we have some.
>>>> > > if (LibraryInfo) FPM.add(new TargetLibraryInfo(*LibraryInfo));
>>>> > >@@ -140,6 +150,9 @@ void
>>>> PassManagerBuilder::populateModulePassManager(PassManagerBase &MPM) {
>>>> > > MPM.add(createBarrierNoopPass());
>>>> > >
>>>> > > addExtensionsToPM(EP_EnabledOnOptLevel0, MPM);
>>>> > >+#if defined(WITH_POLLY) && !defined(POLLY_BUILD_SHARED_LIB)
>>>> > >+ registerPollyOptLevel0Passes(*this, MPM);
>>>> > >+#endif // WITH_POLLY
>
> Is there even a need for this change? I was of the impression, that the
> static initializers in Polly would register it automatically, no?
> Obviously this would require PollyRegisterPasses to be linked in.
Still applies.
>>>> > > return;
>>>> > > }
>>>> > >
>>>> > >diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
>>>> > >index 12e10fd..a3c0875 100644
>>>> > >--- a/tools/CMakeLists.txt
>>>> > >+++ b/tools/CMakeLists.txt
>>>> > >@@ -4,6 +4,11 @@
>>>> > >
>>>> > > 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)
>>>> > >@@ -69,7 +74,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)
>
> Can we always build Polly first? That would make the file simpler, no?
Still applies.
>>>> > >
>>>> > > # Automatically add remaining sub-directories containing a
>>>> 'CMakeLists.txt'
>>>> > > # file as external projects.
>>>> > >--
>>>> > >1.7.6.4
>>>> > >
>>> >
>>>> > > From 7b162776f7fe52fb11a2d61ed4d1833fc1a03bcc Mon Sep 17
>>>> 00:00:00 2001
>>>> > >From: Sebastian Pop<spop at codeaurora.org>
>>>> > >Date: Thu, 31 Oct 2013 07:17:25 -0500
>>>> > >Subject: [PATCH] link polly statically
>>>> > >
>>>> > >---
>>>> > > cmake/FindIsl.cmake | 13 ++++++-
>>>> > > lib/Analysis/CMakeLists.txt | 9 ++++-
>>>> > > lib/Analysis/MayAliasSet.cpp | 42 ++++++++++++++++++++++
>>>> > > lib/CMakeLists.txt | 80
>>>> ++++++++++++++++++++++++++++++------------
>>>> > > lib/CodeGen/CMakeLists.txt | 17 ++++++++-
>>>> > > lib/Exchange/CMakeLists.txt | 19 +++++++++-
>>>> > > lib/JSON/CMakeLists.txt | 7 +++-
>>>> > > lib/MayAliasSet.cpp | 42 ----------------------
>>>> > > lib/RegisterPasses.cpp | 32 +++++++++++-----
>>>> > > lib/Support/CMakeLists.txt | 8 ++++-
>>>> > > test/lit.site.cfg.in | 8 +++-
>>>> > > 11 files changed, 191 insertions(+), 86 deletions(-)
>>>> > > create mode 100644 lib/Analysis/MayAliasSet.cpp
>>>> > > delete mode 100644 lib/MayAliasSet.cpp
>>>> > >
>>>> > >diff --git a/cmake/FindIsl.cmake b/cmake/FindIsl.cmake
>>>> > >index 1c46e72..e3b0701 100644
>>>> > >--- a/cmake/FindIsl.cmake
>>>> > >+++ b/cmake/FindIsl.cmake
>>>> > >@@ -1,12 +1,21 @@
>>>> > >-FIND_PATH(ISL_INCLUDE_DIR isl/val.h)
>>>> > >+IF (POLLY_EXTERNAL_ISL_SOURCE_DIR)
>>>> > >+ add_subdirectory(${POLLY_EXTERNAL_ISL_SOURCE_DIR}
>>>> "${CMAKE_CURRENT_BINARY_DIR}/isl")
>>>> > >+ IF (POLLY_BUILD_SHARED_LIB)
>>>> > >+ SET(ISL_LIBRARY "${ISL_BINARY_DIR}/lib/isl.so")
>>>> > >+ ELSE(POLLY_BUILD_SHARED_LIB)
>>>> > >+ SET(ISL_LIBRARY "${ISL_BINARY_DIR}/lib/isl.a")
>>>> > >+ ENDIF(POLLY_BUILD_SHARED_LIB)
>>>> > >+ SET(ISL_INCLUDE_DIR "${POLLY_EXTERNAL_ISL_SOURCE_DIR}/include")
>>>> > >+ INCLUDE_DIRECTORIES("${POLLY_EXTERNAL_ISL_SOURCE_DIR}/imath")
>>>> > >+ENDIF (POLLY_EXTERNAL_ISL_SOURCE_DIR)
>
> Again, this seems to be an additional feature. I would prefer to 1st
> share, this file between LLVM and Polly and 2nd, to review this in a
> separate patch if possible.
Still applies.
>>>> > >
>>>> > >+FIND_PATH(ISL_INCLUDE_DIR isl/val.h)
>>>> > > FIND_LIBRARY(ISL_LIBRARY NAMES isl)
>>>> > >
>>>> > > IF (ISL_INCLUDE_DIR AND ISL_LIBRARY)
>>>> > > SET(ISL_FOUND TRUE)
>>>> > > ENDIF (ISL_INCLUDE_DIR AND ISL_LIBRARY)
>>>> > >
>>>> > >-
>>>> > > IF (ISL_FOUND)
>>>> > > IF (NOT Isl_FIND_QUIETLY)
>>>> > > MESSAGE(STATUS "Found Isl: ${ISL_LIBRARY}")
>>>> > >diff --git a/lib/Analysis/CMakeLists.txt
>>>> b/lib/Analysis/CMakeLists.txt
>>>> > >index 9e46527..b40fe7b 100644
>>>> > >--- a/lib/Analysis/CMakeLists.txt
>>>> > >+++ b/lib/Analysis/CMakeLists.txt
>>>> > >@@ -1,8 +1,15 @@
>>>> > >-add_polly_library(PollyAnalysis
>>>> > >+set(POLLY_ANALYSIS_SOURCES
>>>> > > Dependences.cpp
>>>> > >+ MayAliasSet.cpp
>
> The move of MayAliasSet seems to be independent of this patch. The
> change is trivial, but large. Please commit this already to the Polly
> branch. This will make this patch set smalled.
Already committed. An updated patchset that does not include this change
will help the review.
>>>> > > ScopDetection.cpp
>>>> > > ScopInfo.cpp
>>>> > > ScopGraphPrinter.cpp
>>>> > > ScopPass.cpp
>>>> > > TempScopInfo.cpp
>>>> > > )
>>>> > >+
>>>> > >+if(POLLY_BUILD_SHARED_LIB)
>>>> > >+ add_polly_library(PollyAnalysis ${POLLY_ANALYSIS_SOURCES})
>>>> > >+else(POLLY_BUILD_SHARED_LIB)
>>>> > >+ add_llvm_library(LLVMPollyAnalysis ${POLLY_ANALYSIS_SOURCES})
>
> Can you help me what the difference here is again? Maybe add a comment
> to explain.
>
> Would it make sense to always use LLVMPollyAnalysis?
As all Polly libraries are now called LLVMPolly*, is this change still
needed?
> 79c5ab 100644
>>>> > >--- a/lib/CMakeLists.txt
>>>> > >+++ b/lib/CMakeLists.txt
>>>> > >@@ -4,31 +4,30 @@ add_subdirectory(Exchange)
>>>> > > add_subdirectory(Support)
>>>> > > add_subdirectory(JSON)
>>>> > >
>>>> > >-set(MODULE TRUE)
>>>> > >+if(POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(MODULE TRUE)
>>>> > >+endif(POLLY_BUILD_SHARED_LIB)
>>>> > > set(LLVM_NO_RTTI 1)
>>>> > >
>>>> > > if (SCOPLIB_FOUND)
>>>> > > set(POLLY_SCOPLIB_FILES Pocc.cpp)
>>>> > >+elseif(NOT POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(LLVM_OPTIONAL_SOURCES ${LLVM_OPTIONAL_SOURCES}
>>>> > >+ Pocc.cpp)
>>>> > > endif (SCOPLIB_FOUND)
>>>> > >
>>>> > > if (PLUTO_FOUND)
>>>> > > set(POLLY_PLUTO_FILES Pluto.cpp)
>>>> > >+elseif(NOT POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(LLVM_OPTIONAL_SOURCES ${LLVM_OPTIONAL_SOURCES}
>>>> > >+ Pluto.cpp)
>>>> > > endif (PLUTO_FOUND)
>
> This change looks also fishy. Can we avoid these conditions.
Still applies.
>>>> > >-if (TARGET intrinsics_gen)
>>>> > >- # Check if we are building as part of an LLVM build
>>>> > >- add_dependencies(LLVMPolly intrinsics_gen)
>>>> > >-endif()
>>>> > >-
>>>> > >-add_dependencies(LLVMPolly
>>>> > >- PollyAnalysis
>>>> > >- PollyCodeGen
>>>> > >- PollyExchange
>>>> > >- PollySupport
>>>> > >- PollyJSON
>>>> > >- )
>>>> > >+if(POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(LLVM_USED_LIBS
>>>> > >+ PollyAnalysis
>>>> > >+ PollyCodeGen
>>>> > >+ PollyExchange
>>>> > >+ PollySupport
>>>> > >+ PollyJSON
>>>> > >+ )
>>>> > >+ add_polly_loadable_module(LLVMPolly ${POLLY_SRC})
>>>> > >+
>>>> > >+ if (TARGET intrinsics_gen)
>>>> > >+ # Check if we are building as part of an LLVM build
>>>> > >+ add_dependencies(LLVMPolly intrinsics_gen)
>>>> > >+ endif()
>>>> > >+
>>>> > >+ add_dependencies(LLVMPolly
>>>> > >+ PollyAnalysis
>>>> > >+ PollyCodeGen
>>>> > >+ PollyExchange
>>>> > >+ PollySupport
>>>> > >+ PollyJSON
>>>> > >+ )
>>>> > >+else(POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(LLVM_USED_LIBS
>>>> > >+ LLVMPollyAnalysis
>>>> > >+ LLVMPollyCodeGen
>>>> > >+ LLVMPollyExchange
>>>> > >+ LLVMPollySupport
>>>> > >+ LLVMPollyJSON
>>>> > >+ )
>
> Before we add complexity to the makefiles, I would rather prefer to
> rename everything to LLVMPolly*. Such a commit (if done for make system
> and cmake) is preapproved for Polly.
After the Polly -> LLVMPolly rename was committed, this should disappear
in the updated patchset.
>>>> > >+ add_llvm_library(LLVMPolly ${POLLY_SRC})
>>>> > >+
>>>> > >+ if (TARGET intrinsics_gen)
>>>> > >+ # Check if we are building as part of an LLVM build
>>>> > >+ add_dependencies(LLVMPolly intrinsics_gen)
>>>> > >+ endif()
>>>> > >+
>>>> > >+ add_dependencies(LLVMPolly
>>>> > >+ LLVMPollyAnalysis
>>>> > >+ LLVMPollyCodeGen
>>>> > >+ LLVMPollyExchange
>>>> > >+ LLVMPollySupport
>>>> > >+ LLVMPollyJSON
>>>> > >+ )
>>>> > >+
>>>> > >+ target_link_libraries(LLVMPolly LLVMTransformUtils LLVMAnalysis)
>>>> > >+endif(POLLY_BUILD_SHARED_LIB)
>
>>>> > > set_target_properties(LLVMPolly
>>>> > > PROPERTIES
>>>> > >diff --git a/lib/CodeGen/CMakeLists.txt b/lib/CodeGen/CMakeLists.txt
>>>> > >index b1c29c5..39c72b0 100644
>>>> > >--- a/lib/CodeGen/CMakeLists.txt
>>>> > >+++ b/lib/CodeGen/CMakeLists.txt
>>>> > >@@ -2,6 +2,10 @@ if (CLOOG_FOUND)
>>>> > > set(CLOOG_FILES
>>>> > > Cloog.cpp
>>>> > > CodeGeneration.cpp)
>>>> > >+elseif(NOT POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(LLVM_OPTIONAL_SOURCES ${LLVM_OPTIONAL_SOURCES}
>>>> > >+ Cloog.cpp
>>>> > >+ CodeGeneration.cpp)
>>>> > > endif (CLOOG_FOUND)
>>>> > >
>>>> > > set(ISL_CODEGEN_FILES
>>>> > >@@ -11,13 +15,22 @@ set(ISL_CODEGEN_FILES
>>>> > > if (GPU_CODEGEN)
>>>> > > set (GPGPU_CODEGEN_FILES
>>>> > > PTXGenerator.cpp)
>>>> > >+elseif(NOT POLLY_BUILD_SHARED_LIB)
>>>> > >+ set(LLVM_OPTIONAL_SOURCES ${LLVM_OPTIONAL_SOURCES}
>>>> > >+ PTXGenerator.cpp)
>>>> > > endif (GPU_CODEGEN)
>
> Sorry, I am a little bit in a rush, but these conditions all over the
> place look surprising. I need to look again into why they are needed.
Still applies.
>>>> > >-add_polly_library(PollyCodeGen
>>>> > >+set(POLLY_CODEGEN_SOURCES
>>>> > > BlockGenerators.cpp
>>>> > > ${CLOOG_FILES}
>>>> > > ${ISL_CODEGEN_FILES}
>>>> > > LoopGenerators.cpp
>>>> > > Utils.cpp
>>>> > > ${GPGPU_CODEGEN_FILES}
>>>> > >-)
>>>> > >+ )
>>>> > >+
>>>> > >+if(POLLY_BUILD_SHARED_LIB)
>>>> > >+ add_polly_library(PollyCodeGen ${POLLY_CODEGEN_SOURCES})
>>>> > >+else(POLLY_BUILD_SHARED_LIB)
>>>> > >+ add_llvm_library(LLVMPollyCodeGen ${POLLY_CODEGEN_SOURCES})
>>>> > >+endif(POLLY_BUILD_SHARED_LIB)
>
> And again, what is the difference here. If we really need to switch
> behavior, we should probably adjust the behavior of add_polly_library
> based on POLLY_BUILD_SHARED_LIB?
Is this diff now obsolete or is some difference left between
add_polly_library and add_llvm_library?
>>>> > >diff --git a/lib/RegisterPasses.cpp b/lib/RegisterPasses.cpp
>>>> > >index 26a138e..bca1cf2 100644
>>>> > >--- a/lib/RegisterPasses.cpp
>>>> > >+++ b/lib/RegisterPasses.cpp
>>>> > >@@ -310,12 +310,9 @@ static void
>>>> registerPollyPasses(llvm::PassManagerBase &PM) {
>>>> > > PM.add(llvm::createCFGPrinterPass());
>>>> > > }
>>>> > >
>>>> > >-static void
>>>> > >-registerPollyEarlyAsPossiblePasses(const
>>>> llvm::PassManagerBuilder &Builder,
>>>> > >- llvm::PassManagerBase &PM) {
>>>> > >-
>>>> > >- if (Builder.OptLevel == 0)
>>>> > >- return;
>>>> > >+static bool shouldEnablePolly(unsigned OptLevel) {
>>>> > >+ if (OptLevel == 0)
>>>> > >+ return false;
>>>> > >
>>>> > > if (PollyOnlyPrinter || PollyPrinter || PollyOnlyViewer ||
>>>> PollyViewer)
>>>> > > PollyTrackFailures = true;
>>>> > >@@ -324,7 +321,16 @@ registerPollyEarlyAsPossiblePasses(const
>>>> llvm::PassManagerBuilder &Builder,
>>>> > > ExportJScop || ImportJScop)
>>>> > > PollyEnabled = true;
>>>> > >
>>>> > >- if (!PollyEnabled)
>>>> > >+ return PollyEnabled;
>>>> > >+}
>>>> > >+
>>>> > >+} // end of anonymous namespace.
>
> Please commit shouldEnablePolly separately. This is a trivial, makes the
> review easier and is preapproved.
Was already committed.
>>>> > >+
>>>> > >+void
>>>> > >+registerPollyEarlyAsPossiblePasses(const
>>>> llvm::PassManagerBuilder &Builder,
>>>> > >+ llvm::PassManagerBase &PM) {
>>>> > >+
>>>> > >+ if (!shouldEnablePolly(Builder.OptLevel))
>>>> > > return;
>>>> > >
>>>> > > // We only run Polly at optimization level '-O3'.
>>>> > >@@ -340,11 +346,15 @@ registerPollyEarlyAsPossiblePasses(const
>>>> llvm::PassManagerBuilder &Builder,
>>>> > > registerPollyPasses(PM);
>>>> > > }
>>>> > >
>>>> > >-static void registerPollyOptLevel0Passes(const
>>>> llvm::PassManagerBuilder &,
>>>> > >- llvm::PassManagerBase
>>>> &PM) {
>>>> > >- registerCanonicalicationPasses(PM);
>>>> > >+void registerPollyOptLevel0Passes(const llvm::PassManagerBuilder
>>>> &Builder,
>>>> > >+ llvm::PassManagerBase &PM) {
>>>> > >+ if (shouldEnablePolly(Builder.OptLevel))
>>>> > >+ registerCanonicalicationPasses(PM);
>>>> > > }
>
> Making those functions non-static without exposing them in a header
> looks like a hack ...
Still applies.
>>>> > >+#ifdef POLLY_BUILD_SHARED_LIB
>>>> > >+namespace {
>>>> > >+
>>>> > > /// @brief Register Polly canonicalization passes at opt level '0'
>>>> > > ///
>>>> > > /// At '-O0' we schedule the Polly canonicalization passes.
>>>> This allows us
>>>> > >@@ -385,3 +395,5 @@ static llvm::RegisterStandardPasses
>>>> > >
>>>> RegisterPollyOptimizer(llvm::PassManagerBuilder::EP_EarlyAsPossible,
>>>> > > registerPollyEarlyAsPossiblePasses);
>>>> > > } // end of anonymous namespace.
>>>> > >+
>>>> > >+#endif // POLLY_BUILD_SHARED_LIB
>
> This static initializer can not be used to register the Polly passes
> even in a statically linked Polly?
Still applies.
> Thanks a lot for your work.
I also tried to build this and it failed for me when using
BUILD_SHARED_LIBS=yes. Otherwise it seems to work.
Cheers,
Tobias
More information about the llvm-commits
mailing list