-
Notifications
You must be signed in to change notification settings - Fork 13.5k
clang 10.0.0_rc1 standalone fails to build with polly #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Can you please provide steps to reproduce, i.e. with what cmake invocations etc. did you build llvm, polly, and clang? |
llvm + polly (polly symlinked into the tools directory) were built with:
Clang is built with
|
I spent some time trying to reproduce this, but with no luck so far. Can you provide more detailed repro steps? This is what I tried:
|
I wonder if that could be because of ThinLTO.
Will try to do that outside of the package manager in a temporary location. |
First test: I unpacked llvm, polly and clang as you did, but only built clang, using the same command as you, apart from llvm-config (using the system one instead, as llvm 10.0.0_rc1 with polly is installed system-wide here). Got the same error:
Fwiw, the
|
I think I know why you weren't able to reproduce. When I wanted to reproduce your full scenario, I just noticed that the three cmake params for polly start with
|
Yup, I confirm that with these typo fixed, I can reproduce the problem with your commands |
The change causing this is probably 24ab9b5. I suspect that Note that |
Building clang separately is at least how all of the source-based distribution do it AFAIK. Do you want me to resubmit the bug to bugs.llvm.org? |
D'oh! Thanks for confirming.
Yes, it would be good to have it there so we can track it as a release blocker. I've filed https://bugs.llvm.org/show_bug.cgi?id=44870 Do you have a bugzilla account so I can cc you? |
I don't (yet) |
Yes, it seems after this change (at 0d27543 which has a build fix), it starts failing to link with undefined reference to `getPollyPluginInfo()' However, I'm also not able to build before that. For example at 24ab9b5^1 my build fails with undefined reference to `polly::initializePollyPasses(llvm::PassRegistry&)' So maybe I'm still not holding the build quite right. My reproduction command was (from a monorepo checkout): $ rm -rf /tmp/issue120 && mkdir /tmp/issue120 && cp -r llvm clang /tmp/issue120/ && cp -r polly /tmp/issue120/llvm/tools/ && ( cd /tmp/issue120 && mkdir build1 && cd build1 && cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/inst -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_POLLY_LINK_INTO_TOOLS=ON -DLLVM_TOOL_POLLY_BUILD=ON -DPOLLY_BUNDLED_ISL=ON ../llvm && ninja && ninja install && cd .. && mkdir build2 && cd build2 && cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/inst -DLLVM_LINK_LLVM_DYLIB=ON -DCLANG_LINK_CLANG_DYLIB=ON -DLLVM_MAIN_SRC_DIR=/tmp/issue120/llvm -DLLVM_CONFIG=/tmp/inst/bin/llvm-config ../clang && ninja clang ) |
For building 9.0.1, we use |
I managed to reproduce the issue and found the actual problem: the extension mechanism is not aware of the DYLIB process, and linked-in extensions are not selected when selecting all components. I'm currently trying a patch, if it works I'll submit it for review. |
This confirms my suspicion that libPolly is just not added into libLLVM.so (the dylib). Before 24ab9b5, the clang executable pulled-in Polly into the executable by calling |
https://reviews.llvm.org/D74464 fixes the issue |
Closed by d21664c |
@serge-sans-paille I don't think that's quite enough. When llvm 10 is installed system-wide and I try to build clang against it, the new added line tries to write to |
Correct! should be fixed by https://reviews.llvm.org/D74602 |
So, just retried the full scenario from above, but using 10.0.0rc2 + applying D74464 and D74602 I still get the initial error:
Here is what I ran:
|
I guess this is because LLVM_COMPILE_EXTENSIONS doesn't contain Polly since add_llvm_pass_plugin from Polly isn't called in standalone clang? |
So, obviously not the right fix, but adding Maybe the final value of |
As suggested in #120, don't try to generate the extension file from clang, only do the linking step. Fixes the regression introduced in D74464 when running cmake inside the clang directory. Differential Revision: https://reviews.llvm.org/D74602
I like that idea, yeah. Let me dig into it later one. I still commitedD74602 as a small step forward. |
Approach implemented in https://reviews.llvm.org/D74757 |
👍 Testing in progress |
Any chance of a backport of these patches to the llvm 10 branch? |
I'm a little bit worried that this is landing so late in the process, and because it's non-trivial cmake changes it has the potential of breaking lots of people. On the other hand, the use of Polly is very limited iiuc, so I'm not sure how bad the bug is. What revisions would need to be ported, and what's your confidence level regarding their correctness? |
So am I. On the other hand there's no regression on master branch as of now, so we already have some positive feedback :-) Concerning the revision that need to be ported, I'd say Concerning their correctness, I can't go further than: bots validation is OK, I tested several configuration locally and @Keruspe did too. Plus this should only impact Polly users, so it should have limited impact. |
I looked at the changes again, and decided not to merge them. It seems to me the risk (cmake breaking somehow) is too high for the reward (standalone clang builds with polly, which isn't widely used). Maybe it can be a candidate for 10.0.1. |
@serge-sans-paille |
Backing out this whole thing would be my preferred approach, but there seem to be too many changes on top to be able to do that cleanly. |
As suggested in llvm/llvm-project#120, don't try to generate the extension file from clang, only do the linking step. Fixes the regression introduced in D74464 when running cmake inside the clang directory. Differential Revision: https://reviews.llvm.org/D74602 (cherry picked from commit 87dac7d)
@llvm/issue-subscribers-polly |
As suggested in llvm/llvm-project#120, don't try to generate the extension file from clang, only do the linking step. Fixes the regression introduced in D74464 when running cmake inside the clang directory. Differential Revision: https://reviews.llvm.org/D74602 (cherry picked from commit 7663149)
…624.3 (llvm#120) [release/11.x] Update dependencies from dotnet/arcade
As suggested in llvm/llvm-project#120, don't try to generate the extension file from clang, only do the linking step. Fixes the regression introduced in D74464 when running cmake inside the clang directory. Differential Revision: https://reviews.llvm.org/D74602
This reverts commit 997f33c. Breaks check-mlir ******************** TEST 'MLIR :: IR/attribute.mlir' FAILED ******************** Script: -- : 'RUN: at line 1'; mlir-opt llvm-project/mlir/test/IR/attribute.mlir -split-input-file -verify-diagnostics | /FileCheck llvm-project/mlir/test/IR/attribute.mlir -- Exit Code: 1 Command Output (stderr): -- llvm-project/mlir/test/IR/attribute.mlir split at line llvm#1:19:3: error: unexpected error: 'test.int_attrs' op requires attribute 'index_attr' "test.int_attrs"() { ^ llvm-project/mlir/test/IR/attribute.mlir split at line llvm#120:6:3: error: unexpected error: 'test.int_attrs' op requires attribute 'index_attr' "test.int_attrs"() { ^ llvm-project/mlir/test/IR/attribute.mlir split at line llvm#120:5:6: error: expected error "'si32_attr' failed to satisfy constraint: 32-bit signed integer attribute" was not produced // expected-error @+1 {{'si32_attr' failed to satisfy constraint: 32-bit signed integer attribute}} ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ llvm-project/mlir/test/IR/attribute.mlir split at line llvm#133:5:3: error: unexpected error: 'test.int_attrs' op requires attribute 'index_attr' "test.int_attrs"() { ^ llvm-project/mlir/test/IR/attribute.mlir split at line llvm#133:4:6: error: expected error "'ui32_attr' failed to satisfy constraint: 32-bit unsigned integer attribute" was not produced // expected-error @+1 {{'ui32_attr' failed to satisfy constraint: 32-bit unsigned integer attribute}} ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ llvm-project/mlir/test/IR/attribute.mlir:9:12: error: CHECK: expected string not found in input // CHECK: any_i32_attr = 5 : ui32 ^ <stdin>:3:1: note: scanning from here module { ^ <stdin>:21:28: note: possible intended match here "test.non_negative_int_attr"() {i32attr = 5 : i32, i64attr = 10 : i64} : () -> ()
Old build issue on old release |
…or mask operations (llvm#120) * [LLVM][XTHeadVector] support `vbool16/32/64` for vector mask operations * [LLVM][XTHeadVector] correctly expand pseudos for `vmset/vmclr` * [LLVM][XTHeadVector] update corresponding tests * [LLVM][XTHeadVector] update tests for `vmsof/vmsbf/vmsif` * [LLVM][XTHeadVector] update tests for `vmfirst/vmpopc` * [LLVM][XTHeadVector] update tests for `vmfirst/vmpopc`
Conditionally skip docs PR build Change-Id: I36e1db0edf58bc2f4ac95686675b315a23fa232a
…464)" (#120511) This reverts commit 2691b96. This reapply fixes the buildbot breakage of the original patch, by updating clang/test/CodeGen/ubsan-trap-debugloc.c to specify -fsanitize-merge (the default, which is merge, is applied by the driver but not clang_cc1). This reapply also expands clang/test/CodeGen/ubsan-trap-merge.c. ---- Original commit message: '-mllvm -ubsan-unique-traps' (#65972) applies to all UBSan checks. This patch introduces -fsanitize-merge (defaults to on, maintaining the status quo behavior) and -fno-sanitize-merge (equivalent to '-mllvm -ubsan-unique-traps'), with the option to selectively applying non-merged handlers to a subset of UBSan checks (e.g., -fno-sanitize-merge=bool,enum). N.B. we do not use "trap" in the argument name since #119302 has generalized -ubsan-unique-traps to work for non-trap modes (min-rt and regular rt). This patch does not remove the -ubsan-unique-traps flag; that will override -f(no-)sanitize-merge.
When llvm is built with polly, clang now fails to build with
undefined symbol: getPollyPluginInfo()
.9.0.1 builds fine
The text was updated successfully, but these errors were encountered: