The header file `llvm/include/llvm/Targetparser/RISCVTargetParser.h` relies on the auto-generated *.inc file associated to the tablegen target `RISCVTargetParserTableGen`. Both clangBasic and clangDriver include `RISCVTargetParser.h`, therefore we need to make sure that the *.inc file is avaiable to avoid compilation errors like the following: FAILED: tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o /usr/bin/c++ [bunch of non interesting stuff] -c <path-to>/llvm-project/clang/lib/Basic/Targets/RISCV.cpp In file included from <path-to>/llvm-project/clang/lib/Basic/Targets/RISCV.cpp:19: <path-to>/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:29:10: fatal error: llvm/TargetParser/RISCVTargetParserDef.inc: No such file or directory 29 | #include "llvm/TargetParser/RISCVTargetParserDef.inc" | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The stand-alone build of `clang` has been tested with the following script (see [*] for further information): ``` build_llvm=`pwd`/build-llvm build_clang=`pwd`/build-clang installprefix=`pwd`/install llvm=`pwd`/llvm-project mkdir -p $build_llvm mkdir -p $installprefix cmake -G Ninja -S $llvm/llvm -B $build_llvm \ -DLLVM_INSTALL_UTILS=ON \ -DCMAKE_INSTALL_PREFIX=$installprefix \ -DCMAKE_BUILD_TYPE=Release ninja -C $build_llvm install cmake -G Ninja -S $llvm/clang -B $build_clang \ -DLLVM_EXTERNAL_LIT=$build_llvm/utils/lit \ -DLLVM_ROOT=$installprefix ``` [*] https://llvm.org/docs/GettingStarted.html#stand-alone-builds Differential Revision: https://reviews.llvm.org/D141581
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@jrtc27, @thakis, @craig.topper - is this a better solution to the issue raised in https://reviews.llvm.org/D137517?
FWIW, the change in this patch solves the issue with standalone build of clang reported in https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf. (FYI, @mgorny )
@jrtc27, @thakis, @craig.topper - gentle ping, it would be great if I could unlock @mgorny with this patch for the issue they are seeing at https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf
Francesco
Why is it not sufficient to link to RISCVTargetParserTableGen, but is sufficient to link to LLVMTargetParser?
Does RISCVTargetParserTableGen itself not link to LLVMTargetParser?
This is because the sources of clangBasic and clangDriver might be compiled before LLVMTargetParser is ready. In this case, compilation would fail because both libraries include the header file llvm/TargetParser/RISCVTargetParser.h, which needs the inc file generated by RISCVTargetParserTableGen.
// quoting code from llvm/TargetParser/RISCVTargetParser.h enum CPUKind : unsigned { #define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM, #define TUNE_PROC(ENUM, NAME) CK_##ENUM, #include "llvm/TargetParser/RISCVTargetParserDef.inc" };
Does RISCVTargetParserTableGen itself not link to LLVMTargetParser?
Nope, there is no "linking" between these two components. It is just that LLVMTargetParser requires RISCVTargetParserTableGen. Therefore, if we say that clangDriver and clangBasic depend on LLVMTargetParser we make sure that the inclusion of the tablegen-generated file resolves correctly.
I wasn't aware of this requirement. As of https://reviews.llvm.org/D137517 it depends on some auto-generated target information stored in LLVMTargetParser and llvm/lib/Target/Target/RISCV/RISCV.td. At the end, it is not even depending on the LLVMTargetParser library, it is just one header file in the public interface of the TargetParser that is needed (see explanation in https://reviews.llvm.org/D141581#4056503)
...
Therefore, if we say that clangDriver and clangBasic depend on LLVMTargetParser we make sure that the inclusion of the tablegen-generated file resolves correctly.
Sorry, I don't follow. If I read correctly, you're saying that clang libraries might begin to compile before their DEPENDS dependency is built (implying that DEPENDS clause only guarantees that the dependency is ready at link stage). If it is true, the proposed patch changes nothing -- the sources might still start to compile before cmake decides to generate inc file, because it is only needed at link stage.
Am I missing something?
Ops, yeah, I think I am wrong. In fact , when RISCVTargetParserTableGen is in the DEPENDS, it should be built before the clangBasic start to compile. This means that I actually do not know why this change fixes the issue reported
@mgorny - can you try this patch on your workflow? Maybe the issue disappeared in my local machine just because for some reason it changed the order of compilation?
@barannikov88 - I am stuck with an incomplete explanation:
The issue raised by @mgorny is about stand-alone builds. In this build configuration, clang is built separately from LLVM, which is first build and installed in a folder.
When clang is build as a stand-alone project, clang does not have access to the cmake configuration of LLVM. Therefore it knows nothing about the tablegen target RISCVTargetParserTableGen.
The's why we see the following error when building stand-alone clang on main:
CMake Error at /Users/fpetrogalli/projects/cpu-defs/install/lib/cmake/llvm/AddLLVM.cmake:536 (add_dependencies): The dependency target "RISCVTargetParserTableGen" of target "obj.clangBasic" does not exist. Call Stack (most recent call first): cmake/modules/AddClang.cmake:106 (llvm_add_library) lib/Basic/CMakeLists.txt:40 (add_clang_library)
I do not know why LLVMTargetParser (which is also specified in the cmake configuration of llvm) is instead recognised as a valid dependencies of clang on some LLVM library. Maybe because cmake looks in the prefix folder where LLVM was installed and finds the object file of the library?
FWIW, I was able to reproduce the issue reported by @mgorny via the following script when run on main:
#!/bin/sh build_llvm=`pwd`/build-llvm build_clang=`pwd`/build-clang installprefix=`pwd`/install llvm=`pwd`/llvm-project mkdir -p $build_llvm mkdir -p $installprefix cmake -G Ninja -S $llvm/llvm -B $build_llvm \ -DLLVM_INSTALL_UTILS=ON \ -DCMAKE_INSTALL_PREFIX=$installprefix \ -DCMAKE_BUILD_TYPE=Release ninja -C $build_llvm install cmake -G Ninja -S $llvm/clang -B $build_clang \ -DLLVM_EXTERNAL_LIT=$build_llvm/utils/llvm-lit \ -DLLVM_ROOT=$installprefix
Given it was an issue that cmake itself was reporting (and not a build time failure), I thought that its disappearance after applying this patch was a signal thatI was doing the right thing.
Ah, I see. The key point is that standalone build that depends on installed LLVM.
RISCVTargetParser is a custom target and it is not being installed, while LLVMTargetParser is a "real" target and gets installed. This is probably why changing the dependency fixes the issue.
The change looks good to me then.
clangBasic and clangDriver already have a dependency on TargetParser (see LLVM_LINK_COMPONENTS at the beginning of corresponding files). Is that not enough?
Will it build if you just remove the additional dependency?
No - if we just specify the dependency in LLVM_LINK_COMPONENTS, there are build failures, as explained in the commit message of https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf.
Hi -has anybody any more concern on this change? I'd like to submit it as soon as possible to unlock @mgorny .
Francesco
It's still not clear to me why LLVM_LINK_COMPONENTS does not work. Is LLVMTargetParser a library?
So, what's the actual dependency here? Do libBasic and libDriver just need the header to be generated or does it actually need to link to the library?
Yes, LLVMTargetParser is a library built located in llvm/lib/TargetParser. LLVM_LINK_COMPONENTS does not work if clangBasic or clangDriver start building before LLVMTargetParser, because the header file llvm/TargetParser/RISCVTargetParserDefs.inc has not been generated yet. This didn't happen on my local build, but on some bot I had the following error reported even if TargetParser was added to LLVM_LINK_COMPONENTS.
<path-to>/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:29:10: fatal error: llvm/TargetParser/RISCVTargetParserDef.inc: No such file or directory
29 | #include "llvm/TargetParser/RISCVTargetParserDef.inc" | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
By adding LLVMTargetParser to the DEPENDS of clangBasic and clangDriver it seems that the order of building LLVMTargetParser before any of the dependents is enforced.
To put in other words, it seems that just specifying` LINK_COMPONENTS = A` for a library B allows A and B to be built in parallel.
I don't think this is the correct way to specify dependencies if it's just an issue of the header being included before a generated file it needs has been generated. Are there other places in the code where a generated header file is included by another header?
% grep -r "RISCVTargetParser.h" * include/llvm/module.modulemap: header "TargetParser/RISCVTargetParser.h" lib/Target/RISCV/RISCVISelLowering.h:#include "llvm/TargetParser/RISCVTargetParser.h" lib/TargetParser/RISCVTargetParser.cpp:#include "llvm/TargetParser/RISCVTargetParser.h"
RISCVTargetParser.h is the one that references the generated file via :
enum CPUKind : unsigned { #define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM, #define TUNE_PROC(ENUM, NAME) CK_##ENUM, #include "llvm/TargetParser/RISCVTargetParserDef.inc" };
I spent some time looking at this. Rather than changing the dependency from RISCVTargetParserTableGen to LLVMTargetParser, I think the correct fix is to handle RISCVTargetParserTableGen in llvm/cmake/modules/LLVMConfig.cmake.in. alongside the intrinsics_gen, omp_gen, and acc_gen targets.
@tstellar as you suspected, I had the following problem when building clang in standalone mode and your patch solved it for me. Thank you.
-- Configuring done CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:536 (add_dependencies): The dependency target "RISCVTargetParserTableGen" of target "obj.clangBasic" does not exist. Call Stack (most recent call first): cmake/modules/AddClang.cmake:106 (llvm_add_library) lib/Basic/CMakeLists.txt:40 (add_clang_library) CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:719 (add_dependencies): The dependency target "RISCVTargetParserTableGen" of target "clangBasic" does not exist. Call Stack (most recent call first): cmake/modules/AddClang.cmake:106 (llvm_add_library) lib/Basic/CMakeLists.txt:40 (add_clang_library) CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:536 (add_dependencies): The dependency target "RISCVTargetParserTableGen" of target "obj.clangDriver" does not exist. Call Stack (most recent call first): cmake/modules/AddClang.cmake:106 (llvm_add_library) lib/Driver/CMakeLists.txt:17 (add_clang_library) CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:719 (add_dependencies): The dependency target "RISCVTargetParserTableGen" of target "clangDriver" does not exist. Call Stack (most recent call first): cmake/modules/AddClang.cmake:106 (llvm_add_library) lib/Driver/CMakeLists.txt:17 (add_clang_library)
This is currently holding back further testing on our end which is concerning me a bit, especially as we approach the branch point. Could you revert this please if a fix isn't imminent? Thank you!
@thesamesam what do you mean with "revert"? Just by looking at this differential I cannot see that this patch has landed. Has it?
He was referring to reverting ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf, i.e. the change that introduced the problem. However, FWICS reverting it is non-trivial and will probably cause quite a mess.
So, well, we're stuck for two weeks now being unable to test new LLVM versions, the branching point is approaching fast and it's quite likely we'll have to put a lot of effort afterwards to fix everything and request backporting our fixes to 16.x.
@mgorny, I have updated the patch according to the suggestions from @tstellar in https://reviews.llvm.org/D141581#4069857.
I have used the script below to test the stand-alone build of clang. Indeed, the error related to the cmake configuration for RISCVTargetParserDefs has disappeared. However, there is an (unreltaed) error that does not allow me to test the full build:
CMake Error at /Users/fpetrogalli/projects/cpu-defs/install/lib/cmake/llvm/LLVMExports.cmake:1222 (set_target_properties): The link interface of target "LLVMLineEditor" contains: LibEdit::LibEdit but the target was not found. Possible reasons include: * There is a typo in the target name. * A find_package call is missing for an IMPORTED target. * An ALIAS target is missing. Call Stack (most recent call first): /Users/fpetrogalli/projects/cpu-defs/install/lib/cmake/llvm/LLVMConfig.cmake:329 (include) CMakeLists.txt:38 (find_package)
Script used for testing:
% cat standalone.sh #!/bin/sh build_llvm=`pwd`/build-llvm build_clang=`pwd`/build-clang installprefix=`pwd`/install llvm=`pwd`/llvm-project mkdir -p $build_llvm mkdir -p $installprefix cmake -G Ninja -S $llvm/llvm -B $build_llvm \ -DLLVM_INSTALL_UTILS=ON \ -DCMAKE_INSTALL_PREFIX=$installprefix \ -DCMAKE_BUILD_TYPE=Release ninja -C $build_llvm install cmake -G Ninja -S $llvm/clang -B $build_clang \ -DLLVM_EXTERNAL_LIT=$build_llvm/utils/lit \ -DLLVM_ROOT=$installprefix
clang/lib/Basic/CMakeLists.txt | ||
---|---|---|
113 | Doesn't this break the monorepo (non-standalone) build? If the monorepo build still works with this, then you don't need the changes to llvm/cmake/modules/LLVMConfig.cmake.in |
I tested this patch and it works, so I'll give it an LGTM. However, I think the LLVMConfig.cmake.in changes are now unnecessary since the RISCVTargetParserTableGen has been dropped from all clang targets.
clang/lib/Basic/CMakeLists.txt | ||
---|---|---|
113 | This configuration built without problems on my machine. cmake ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release -G Ninja |
I am tempted to submit it as it is and create a new patch that removes the bits in llvm/cmake/modules/LLVMConfig.cmake.in. As far s I can tell, if we specify the library dependency only in LLVM_LINK_COMPONENTS, there are issue when building non stand-alone clang, as building the clang components might start before the actual header file is generated. I'll submit this tomorrow (if I don't get negative feedback), and then create a new patch that removes the bits in llvm/cmake/modules/LLVMConfig.cmake.in.
@fpetrogalli Please go ahead and push this since it is blocking others, we can fix up any of the other issues later.
Ah - there you go. Without a proper chained dependency of LLVMTargetParser -> clangBasic we end up with failures if clangBasic is compiled before LLVMTargetParser is (even on non stand-alone builds). https://lab.llvm.org/buildbot/#/builders/193/builds/25362 :
ccache /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.build/tools/clang/lib/Basic -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/lib/Basic -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/include -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.build/tools/clang/include -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.build/include -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o -MF tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o.d -o tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o -c /home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/lib/Basic/Targets/RISCV.cpp In file included from /home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/lib/Basic/Targets/RISCV.cpp:19: /home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/llvm/include/llvm/TargetParser/RISCVTargetParser.h:29:10: fatal error: llvm/TargetParser/RISCVTargetParserDef.inc: No such file or directory 29 | #include "llvm/TargetParser/RISCVTargetParserDef.inc" | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. 2.225 [3029/31/825] Building CXX object tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Version.cpp.o 2.524 [3029/30/826] Building RISCVTargetParserDef.inc...
I'll revert D141581, as it has chance to fail more bots than the original code.
FWIW, I am out of options. We need to make sure that LLVMTargetParser (or just RISCVTargetParserTablegen) are built - or better, the header file llvm/include/llvm/TargetParser/RISCVTargetParser.h is fully available with its .inc inclusion _before_ clangBasic and clangDriver are compiled.
This patch shows that just specifying the dependency of TargetParser in LLVM_LINK_COMPONENTS is not enough: we need to specify in the DEPENDS on clangDriver and clangBasic the tablegen target or LLVMTargetParser itself.
What I do not understand is why adding DEPENDS LLVMTargetParser (which seems to fix both stand-alone and not) to the clang libraries is not acceptable.
@fpetrogalli The solution is to add RISCVTargetParserTableGen to the Depnds list of clang/lib/Driver and clang/lib/Basic.
Doesn't this break the monorepo (non-standalone) build? If the monorepo build still works with this, then you don't need the changes to llvm/cmake/modules/LLVMConfig.cmake.in