[polly] static builds
Sebastian Pop
spop at codeaurora.org
Fri Mar 7 09:29:33 PST 2014
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.
Can we work together incrementally from these patches to address the missing
bits in initializing Polly passes?
Thanks,
Sebastian
Tobias Grosser wrote:
> 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
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-link-polly-statically.patch
Type: text/x-diff
Size: 2159 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140307/88c667c5/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-static-link-polly.patch
Type: text/x-diff
Size: 3588 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140307/88c667c5/attachment-0001.patch>
More information about the llvm-commits
mailing list