This is a conversion from https://reviews.llvm.org/D89142 to an unsplit declarative model.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Merged with part one of the patch
Remove unnecessary component option
Cleanup layout
Remove obsolete documentation
Now ready for review
Thanks for your effort. I love it. Seems to remove complexities of the buildsystem and the ugly split of having to define library dependencies in two files.
If needed, I can check whether anything break on Windows.
llvm/cmake/modules/AddLLVM.cmake | ||
---|---|---|
622 | SO = significant other? | |
636–637 | [nit] unrelated whitespace change | |
648–653 | Is there somewhere an overview/description of these target properties? | |
llvm/lib/ExecutionEngine/MCJIT/CMakeLists.txt | ||
7 | Could you put the dependees on separate line, like DEPENDS options above? |
Add more comments / documentation
Address review, including reformatting to follow the « one line per argument » layout
This looks like a major simplification of the build system. Code looks good, I confirmed it is working for my Windows build, Polly (i.e. statically linked pass plugins) is listed with --libs.
Since it is a relatively invasive change, you might want a second acceptance as well.
llvm/cmake/modules/AddLLVM.cmake | ||
---|---|---|
636–637 | [nit] still unrelated whitespace change | |
641 | [typo] reseolve | |
733 | [typo] article missing: a component group | |
734 | [typo] components | |
737 | Could you document here that ADD_TO_COMPONENT of add_llvm_component_library is used to add a component to this group? | |
751 | [type] speicified | |
llvm/cmake/modules/LLVM-Build.cmake | ||
42 | [nit] stray whitespace |
Hi, it looks like this might be leading to some test failures on our toolchain builders:
******************** Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90 FAIL: LLVM :: tools/llvm-config/booleans.test (37813 of 40210) ******************** TEST 'LLVM :: tools/llvm-config/booleans.test' FAILED ******************** Script: -- : 'RUN: at line 2'; /b/s/w/ir/x/w/staging/llvm_build/bin/llvm-config --assertion-mode 2>&1 | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --check-prefix=CHECK-ONOFF /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test : 'RUN: at line 8'; /b/s/w/ir/x/w/staging/llvm_build/bin/llvm-config --has-rtti 2>&1 | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --check-prefix=CHECK-YESNO /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test : 'RUN: at line 14'; /b/s/w/ir/x/w/staging/llvm_build/bin/llvm-config --build-mode 2>&1 | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --check-prefix=CHECK-BUILD-MODE /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test : 'RUN: at line 19'; /b/s/w/ir/x/w/staging/llvm_build/bin/llvm-config --build-system 2>&1 | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --check-prefix=CHECK-BUILD-SYSTEM /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test : 'RUN: at line 24'; /b/s/w/ir/x/w/staging/llvm_build/bin/llvm-config --shared-mode 2>&1 | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --check-prefix=CHECK-SHARED-MODE /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test -- Exit Code: 1 Command Output (stderr): -- /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test:26:24: error: CHECK-SHARED-MODE-NOT: excluded string found in input CHECK-SHARED-MODE-NOT: error: ^ <stdin>:3:14: note: found here llvm-config: error: missing: /b/s/w/ir/x/w/staging/llvm_build/lib/libLLVMFrontendOpenACC.a ^~~~~~ Input file: <stdin> Check file: /b/s/w/ir/x/w/llvm-project/llvm/test/tools/llvm-config/booleans.test -dump-input=help explains the following input dump. Input was: <<<<<< 1: llvm-config: error: component libraries and shared library 2: 3: llvm-config: error: missing: /b/s/w/ir/x/w/staging/llvm_build/lib/libLLVMFrontendOpenACC.a not:26 !~~~~~ error: no match expected >>>>>> -- ******************** Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. ******************** Failed Tests (2): LLVM :: tools/llvm-config/booleans.test LLVM :: tools/llvm-config/system-libs.test
Since it seems that this involves just build system changes, do you know if this is something that should be fixed on our end or there's something this patch overlooked? Thanks.
Builder: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-x64/b8863755892480463712
Since it seems that this involves just build system changes, do you know if this is something that should be fixed on our end or there's something this patch overlooked? Thanks.
Builder: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-x64/b8863755892480463712
There was an inconsistency in the LLVMBuild.txt for LLVM Frontend: OpenACC wasn't listed but it was part of the CMake infrastructure. A side effect of the llvmbuildectomy has been to remove that inconsistency and register OpenACC as a regular component.
Looking at the error log, there is a missing libLLVMFrontendOpenACC.a
But I cannot see any reference to FrontendOpenACC in the build log, just as if it wasn't build, which is odd.
I failed to reproduce your issue, but does that OpenACC stuff rings any bell? Can you force building that component and see what happens?
Such inconsistencies and avoiding those in the future were why I strongly preferred this version over llvmbuildectomy version (D89142). OpenACC was probably just forgotten in D83649. Ping @clementval who authored this patch.
There was an inconsistency in the LLVMBuild.txt for LLVM Frontend: OpenACC wasn't listed but it was part of the CMake infrastructure. A side effect of the llvmbuildectomy has been to remove that inconsistency and register OpenACC as a regular component.
Looking at the error log, there is a missing libLLVMFrontendOpenACC.a
But I cannot see any reference to FrontendOpenACC in the build log, just as if it wasn't build, which is odd.
I failed to reproduce your issue, but does that OpenACC stuff rings any bell? Can you force building that component and see what happens?
Forcing a build of OpenACC then re-running the test fixes my issue. Is there perhaps someway in cmake to indicate that this library should just automatically be built whenever I run ninja check-llvm?
My cmake invocation also if it helps is:
cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=/usr/local/google/home/leonardchan/fuchsia/prebuilt/third_party/clang/linux-x64/bin/clang -DCMAKE_CXX_COMPILER=/usr/local/google/home/leonardchan/fuchsia/prebuilt/third_party/clang/linux-x64/bin/clang++ -DLLVM_ENABLE_LLD=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_ENABLE_LIBCXX=On /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-5/llvm
followed by LIT_FILTER=booleans.test ninja check-llvm.
Are there any OpenACC specific tests?
AFAICT after a quick look through the monorepo, there doesn't seem to be any llvm-specific tests for OpenACC.
It fails for me as well: configuring just LLVM alone and running ninja check-llvm instead of building first everything with ninja alone.
OpenACC specific tests are part of Flang or MLIR for the OpenACC dialect. The frontend part should be similar to OpenMP.
llvm/cmake/modules/LLVM-Build.cmake | ||
---|---|---|
85 | Had some build failures downstream after bringing in this patch. I kind of needed this (not sure it is exactly what you wanna do): diff --git a/llvm/cmake/modules/LLVM-Build.cmake b/llvm/cmake/modules/LLVM-Build.cmake index 746f09b09019..21818587766a 100644 --- a/llvm/cmake/modules/LLVM-Build.cmake +++ b/llvm/cmake/modules/LLVM-Build.cmake @@ -82,7 +82,9 @@ endfunction() # Resolve cross-component dependencies, for each available component. function(LLVMBuildResolveComponentsLink) get_property(llvm_components GLOBAL PROPERTY LLVM_COMPONENT_LIBS) - get_property(llvm_has_jit_native TARGET ${LLVM_NATIVE_ARCH} PROPERTY LLVM_HAS_JIT) + if(have_native_backend) + get_property(llvm_has_jit_native TARGET ${LLVM_NATIVE_ARCH} PROPERTY LLVM_HAS_JIT) + endif() if(llvm_has_jit_native) set_property(TARGET Engine APPEND PROPERTY LLVM_LINK_COMPONENTS "MCJIT" "Native") else() otherwise I get CMake Error at cmake/modules/LLVM-Build.cmake:85 (get_property): get_property could not find TARGET X86. Perhaps it has not yet been created. Call Stack (most recent call first): lib/CMakeLists.txt:53 (LLVMBuildResolveComponentsLink) when for example doing cmake like this cmake /my-llvm-repo/llvm --debug-trycompile -G Ninja -DCMAKE_MAKE_PROGRAM=/ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX=/compiler-clang -DLLVM_ENABLE_WERROR=ON -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra' -DLLVM_TARGETS_TO_BUILD='MyOOTTarget' -DLLVM_ENABLE_DOXYGEN=ON |
llvm/cmake/modules/LLVM-Build.cmake | ||
---|---|---|
85 | I think this got fixed by f4a4c63588d4d784f42a94591c46b72bec64a415. Can you confirm? |
llvm/cmake/modules/LLVM-Build.cmake | ||
---|---|---|
85 | Ah...oh... yes that seem to solve the problem we had. (Sorry, started up Monday morning by looking at the automated things that had been running during the weekend. Stupid enough not to try doing another fetch before complaining here. Never mind then, just me being too slow rebasing to latest.) |
llvm/cmake/modules/LLVM-Build.cmake | ||
---|---|---|
85 | I'd rather have three false positive rather than a silent true positive :-) |
SO = significant other?