Page MenuHomePhabricator

Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.
AbandonedPublic

Authored by DiamondLovesYou on Sep 12 2018, 7:20 AM.

Details

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

Diff Detail

Event Timeline

Fix cmake warning

beanz added a comment.Sep 12 2018, 5:19 PM

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 added inline comments.
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.

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 abandoned this revision.Sep 13 2018, 11:16 AM

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).

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 <wichard@vitalitystudios.com> wrote:

On Thu, Sep 13, 2018 at 12:19 PM Roman Lebedev <lebedev.ri@gmail.com> 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 added a comment.Sep 13 2018, 1:50 PM

@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.

+ # 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, 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.

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.