Page MenuHomePhabricator

llvmbuildectomy - part 2
ClosedPublic

Authored by serge-sans-paille on Thu, Nov 5, 6:15 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 5, 6:15 AM
serge-sans-paille requested review of this revision.Thu, Nov 5, 6:15 AM
serge-sans-paille retitled this revision from [WIP] llvmbuildectomy - part 2 to llvmbuildectomy - part 2.
serge-sans-paille edited the summary of this revision. (Show Details)

Merged with part one of the patch
Remove unnecessary component option
Cleanup layout
Remove obsolete documentation

Now ready for review

Add the 'all' group component

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?

serge-sans-paille marked 4 inline comments as done.

Add more comments / documentation
Address review, including reformatting to follow the « one line per argument » layout

llvm/cmake/modules/AddLLVM.cmake
622

The comment is not mine, I'd guess *Shared Objects*

648–653

I've added a description of the public properties in add_llvm_component_library and commented on private properties inplace.

llvm/lib/ExecutionEngine/MCJIT/CMakeLists.txt
7

Done for all the files

Meinersbur accepted this revision.Tue, Nov 10, 4:26 PM

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

This revision is now accepted and ready to land.Tue, Nov 10, 4:26 PM

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.

cc @mehdi_amini or @GMNGeoffrey ?

Address various nits

lenary removed a subscriber: lenary.Fri, Nov 13, 3:55 AM

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?

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.

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.

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?

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.

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.

OpenACC specific tests are part of Flang or MLIR for the OpenACC dialect. The frontend part should be similar to OpenMP.

bjope added a subscriber: bjope.Mon, Nov 16, 1:41 AM
bjope added inline comments.
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?

bjope added inline comments.Mon, Nov 16, 3:57 AM
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 :-)