Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.

[DiamondLovesYou]

Summary

bugpoint/opt doesn't need to link to Polly when using the dylib, which will already contain Polly.

Fix a few other tools when using LLVM_LINK_LLVM_DYLIB

[DiamondLovesYou]

Fix cmake warning

[beanz]

I think this should be split into multiple patches. The places you changed add_library to add_llvm_library should be split out. Also you can break out the unittests/Passes/CmakeLists.txt into its own patch. All of those are good cleanup, and you can commit them without review.

I have the same concern with the bugpoint part of the patch as I do with your clang driver patch. I’m not at my computer, but I can probably take some time tomorrow to look at what it would take to teach llvm_map_components_to_libnames, and llvm_config to special case Polly.

[Meinersbur]

tools/bugpoint/CMakeLists.txt

40

Thanks for taking care of this. Compiling the different ways Polly can be configured (as loadable module/part of LLVM; static/BUILD_SHARED_LIBS/DYLIB; inside the LLVM build tree/standalone; llvm-config/CMake module) is surprisingly hard and we probably did not test all combinations.

[beanz]

If you add this to your patch here, the Clang patch should work as I've suggested:

diff --git a/cmake/modules/LLVM-Config.cmake b/cmake/modules/LLVM-Config.cmake
index 8eabddc7377..a306e41f26d 100644
--- a/cmake/modules/LLVM-Config.cmake
+++ b/cmake/modules/LLVM-Config.cmake
@@ -244,6 +244,9 @@ function(llvm_map_components_to_libnames out_libs)
           list(APPEND expanded_components "LLVM${t}Info")
         endif()
       endforeach(t)
+    elseif( c STREQUAL "Polly")
+      # LLVMPolly is the Polly loadable module target, the static archive is just Polly
+      list(APPEND expanded_components "${c}")
     else( NOT idx LESS 0 )
       # Canonize the component name:
       string(TOUPPER "${c}" capitalized)

It is unfortunate that Polly doesn't match the naming conventions of other LLVM components, but we have a lot of special case handling for this kind of thing anyways.

[DiamondLovesYou]

In D51984#1233719, @beanz wrote:

If you add this to your patch here, the Clang patch should work as I've suggested:

diff --git a/cmake/modules/LLVM-Config.cmake b/cmake/modules/LLVM-Config.cmake
index 8eabddc7377..a306e41f26d 100644
--- a/cmake/modules/LLVM-Config.cmake
+++ b/cmake/modules/LLVM-Config.cmake
@@ -244,6 +244,9 @@ function(llvm_map_components_to_libnames out_libs)
           list(APPEND expanded_components "LLVM${t}Info")
         endif()
       endforeach(t)
+    elseif( c STREQUAL "Polly")
+      # LLVMPolly is the Polly loadable module target, the static archive is just Polly
+      list(APPEND expanded_components "${c}")
     else( NOT idx LESS 0 )
       # Canonize the component name:
       string(TOUPPER "${c}" capitalized)

It is unfortunate that Polly doesn't match the naming conventions of other LLVM components, but we have a lot of special case handling for this kind of thing anyways.

Thanks! I actually already had a similar patch ready to go, but the yelling from the buildbots distracted me (yikes; can't we have a rust-lang like process for this? Like buildbots first? It's pretty hard to get all this right everywhere... especially for a noob like me). I'll get that up on Phab Soon(TM).

[lebedev.ri]

In D51984#1233755, @DiamondLovesYou wrote:

In D51984#1233719, @beanz wrote:

If you add this to your patch here, the Clang patch should work as I've suggested:

diff --git a/cmake/modules/LLVM-Config.cmake b/cmake/modules/LLVM-Config.cmake
index 8eabddc7377..a306e41f26d 100644
--- a/cmake/modules/LLVM-Config.cmake
+++ b/cmake/modules/LLVM-Config.cmake
@@ -244,6 +244,9 @@ function(llvm_map_components_to_libnames out_libs)
           list(APPEND expanded_components "LLVM${t}Info")
         endif()
       endforeach(t)
+    elseif( c STREQUAL "Polly")
+      # LLVMPolly is the Polly loadable module target, the static archive is just Polly
+      list(APPEND expanded_components "${c}")
     else( NOT idx LESS 0 )
       # Canonize the component name:
       string(TOUPPER "${c}" capitalized)

It is unfortunate that Polly doesn't match the naming conventions of other LLVM components, but we have a lot of special case handling for this kind of thing anyways.

Thanks! I actually already had a similar patch ready to go, but the yelling from the buildbots distracted me (yikes; can't we have a rust-lang like process for this? Like buildbots first? It's pretty hard to get all this right everywhere... especially for a noob like me). I'll get that up on Phab Soon(TM).

On Thu, Sep 13, 2018 at 8:22 PM, Richard Diamond <[e-mail]> wrote:

On Thu, Sep 13, 2018 at 12:19 PM Roman Lebedev <[e-mail]> wrote:

Richard, why are you making non-NFC cmake changes with no review all
over the place?

I apologize. I was informed by Chris (beanz) that the llvm-cfi-verify/etc
changes could be pushed without review. The second patch was to fix an error
on my part (missing intrinsics_gen in a target's DEPENDS) which blew up
the bots.

@DiamondLovesYou i would *strongly* suggest that you read through
https://llvm.org/docs/DeveloperPolicy.html
and
https://llvm.org/docs/Phabricator.html
a few times.

[beanz]

@lebedev.ri, I told him he could commit the changes without review because I saw them in this patch, and in my mind, as an experienced developer in this area of LLVM, I saw those changes as obvious and good. That is not in violation of the developer policy, further that discussion is right here in this review, so if you have a problem with it you should point it at me, not @DiamondLovesYou.

[Meinersbur]

In D51984#1233719, @beanz wrote:

+ # LLVMPolly is the Polly loadable module target, the static archive is just Polly
It is unfortunate that Polly doesn't match the naming conventions of other LLVM components, but we have a lot of special case handling for this kind of thing anyways.

This is historic: At the beginning there was just "LLVMPolly", the loadable module (probably to match the convention). When linking as a static component of LLVM, "LLVMPolly" was taken, so it was called just "Polly".
If @grosser agrees, we can switch the naming (with the problem that the command line -load LLVMPolly.so won't work anymore).

[lebedev.ri]

In D51984#1234016, @beanz wrote:

@lebedev.ri, I told him he could commit the changes without review because I saw them in this patch, and in my mind, as an experienced developer in this area of LLVM, I saw those changes as obvious and good. That is not in violation of the developer policy, further that discussion is right here in this review, so if you have a problem with it you should point it at me, not @DiamondLovesYou.

It was the commit messages that threw me off, i wasn't really affected by the fallout of the change.

[grosser]

Thank you guys for taking care of some of the cmake system. @Meinersbur, I am fine with renaming Polly modules according to LLVM conventions if this makes things more consistent.

tools/bugpoint/CMakeLists.txt

add_llvm_tool(bugpoint
   ToolRunner.cpp
   bugpoint.cpp
 
   DEPENDS
   intrinsics_gen
   )
 export_executable_symbols(bugpoint)
 
-if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS AND NOT LLVM_LINK_LLVM_DYLIB)
   target_link_libraries(bugpoint PRIVATE Polly)
   # Ensure LLVMTarget can resolve dependences in Polly.
   target_link_libraries(bugpoint PRIVATE LLVMTarget)
-endif(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+endif()

tools/llvm-cfi-verify/lib/CMakeLists.txt

-add_library(LLVMCFIVerify
-  STATIC
+add_llvm_library(LLVMCFIVerify
   FileAnalysis.cpp
   FileAnalysis.h
   GraphBuilder.cpp
   GraphBuilder.h
-  )
 
-llvm_update_compile_flags(LLVMCFIVerify)
-llvm_map_components_to_libnames(libs
+  STATIC
+
+  LINK_COMPONENTS
   DebugInfoDWARF
   MC
   MCParser
   Object
   Support
   Symbolize
   )
-target_link_libraries(LLVMCFIVerify ${libs})
-set_target_properties(LLVMCFIVerify PROPERTIES FOLDER "Libraries")

tools/llvm-exegesis/lib/AArch64/CMakeLists.txt

 include_directories(
   ${LLVM_MAIN_SRC_DIR}/lib/Target/AArch64
   ${LLVM_BINARY_DIR}/lib/Target/AArch64
   )
 
-add_library(LLVMExegesisAArch64
-  STATIC
+add_llvm_library(LLVMExegesisAArch64
   Target.cpp
-  )
 
-llvm_update_compile_flags(LLVMExegesisAArch64)
-llvm_map_components_to_libnames(libs
+  STATIC
+
+  LINK_COMPONENTS
   AArch64
   Exegesis
   )
-
-target_link_libraries(LLVMExegesisAArch64 ${libs})
-set_target_properties(LLVMExegesisAArch64 PROPERTIES FOLDER "Libraries")

tools/llvm-exegesis/lib/CMakeLists.txt

 if (LLVM_TARGETS_TO_BUILD MATCHES "X86")
   add_subdirectory(X86)
   set(LLVM_EXEGESIS_TARGETS "${LLVM_EXEGESIS_TARGETS} X86" PARENT_SCOPE)
 endif()
 if (LLVM_TARGETS_TO_BUILD MATCHES "AArch64")
   add_subdirectory(AArch64)
   set(LLVM_EXEGESIS_TARGETS "${LLVM_EXEGESIS_TARGETS} AArch64" PARENT_SCOPE)
 endif()
 
-add_library(LLVMExegesis
-  STATIC
+set(libs)
+if(LLVM_ENABLE_LIBPFM AND HAVE_LIBPFM)
+  list(APPEND libs pfm)
+endif()
+
+add_llvm_library(LLVMExegesis
   Analysis.cpp
   Assembler.cpp
   BenchmarkResult.cpp
   BenchmarkRunner.cpp
   Clustering.cpp
   Latency.cpp
   LlvmState.cpp
   MCInstrDescView.cpp
   PerfHelper.cpp
   RegisterAliasing.cpp
   Target.cpp
   Uops.cpp
-  )
 
-llvm_update_compile_flags(LLVMExegesis)
-llvm_map_components_to_libnames(libs
+  STATIC
+
+  LINK_COMPONENTS
   Analysis
   CodeGen
   Core
   ExecutionEngine
   GlobalISel
   MC
   MCDisassembler
   MCJIT
   Object
   ObjectYAML
   Support
-  )
 
-if(LLVM_ENABLE_LIBPFM AND HAVE_LIBPFM)
-  list(APPEND libs pfm)
-endif()
-
-target_link_libraries(LLVMExegesis ${libs})
-set_target_properties(LLVMExegesis PROPERTIES FOLDER "Libraries")
+  LINK_LIBS ${libs}
+  )

tools/llvm-exegesis/lib/X86/CMakeLists.txt

 include_directories(
   ${LLVM_MAIN_SRC_DIR}/lib/Target/X86
   ${LLVM_BINARY_DIR}/lib/Target/X86
   )
 
-add_library(LLVMExegesisX86
-  STATIC
+add_llvm_library(LLVMExegesisX86
   Target.cpp
-  )
 
-llvm_update_compile_flags(LLVMExegesisX86)
-llvm_map_components_to_libnames(libs
+  STATIC
+
+  LINK_COMPONENTS
   X86
   Exegesis
   )
-
-target_link_libraries(LLVMExegesisX86 ${libs})
-set_target_properties(LLVMExegesisX86 PROPERTIES FOLDER "Libraries")

tools/llvm-mca/lib/CMakeLists.txt

 include_directories(${LLVM_MCA_SOURCE_DIR}/include)
 
-add_library(LLVMMCA
-  STATIC
+add_llvm_library(LLVMMCA
   Context.cpp
   HWEventListener.cpp
   HardwareUnits/HardwareUnit.cpp
   HardwareUnits/LSUnit.cpp
   HardwareUnits/RegisterFile.cpp
   HardwareUnits/ResourceManager.cpp
   HardwareUnits/RetireControlUnit.cpp
   HardwareUnits/Scheduler.cpp
   InstrBuilder.cpp
   Instruction.cpp
   Pipeline.cpp
   Stages/DispatchStage.cpp
   Stages/ExecuteStage.cpp
   Stages/FetchStage.cpp
   Stages/InstructionTables.cpp
   Stages/RetireStage.cpp
   Stages/Stage.cpp
   Support.cpp
-  )
 
-llvm_update_compile_flags(LLVMMCA)
-llvm_map_components_to_libnames(libs
+  STATIC
+
+  LINK_COMPONENTS
   CodeGen
   MC
   Support
   )
-
-target_link_libraries(LLVMMCA ${libs})
-set_target_properties(LLVMMCA PROPERTIES FOLDER "Libraries")

tools/opt/CMakeLists.txt

add_llvm_tool(opt
   PrintSCC.cpp
   opt.cpp
 
   DEPENDS
   intrinsics_gen
   )
 export_executable_symbols(opt)
 
-if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS AND NOT LLVM_LINK_LLVM_DYLIB)
   target_link_libraries(opt PRIVATE Polly)
-endif(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+endif()

unittests/Passes/CMakeLists.txt

 # Needed by LLVM's CMake checks because this file defines multiple targets.
 set(LLVM_OPTIONAL_SOURCES PluginsTest.cpp TestPlugin.cpp)
 
 # If plugins are disabled, this test will disable itself at runtime. Otherwise,
 # reconfiguring with plugins disabled will leave behind a stale executable.
 if (LLVM_ENABLE_PLUGINS)
   add_definitions(-DLLVM_ENABLE_PLUGINS)
 endif()
 
 set(LLVM_LINK_COMPONENTS Support Passes Core)
 add_llvm_unittest(PluginsTests
   PluginsTest.cpp
+
+  DEPENDS
+  TestPlugin
   )
 export_executable_symbols(PluginsTests)
 
-set(LLVM_LINK_COMPONENTS)
 add_llvm_loadable_module(TestPlugin
   TestPlugin.cpp
+
+  DEPENDS
+  intrinsics_gen
   )
 
 # Put plugin next to the unit test executable.
 set_output_directory(TestPlugin
   BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
   LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
   )
 set_target_properties(TestPlugin PROPERTIES FOLDER "Tests")
-
-add_dependencies(TestPlugin intrinsics_gen)
-add_dependencies(PluginsTests TestPlugin)