Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Yeah, it sure looks like we're tablegen-ing something and not attaching it to a CMake target... The idea of this seems correct.
You're making LLVMAsmParser depend on this which looks correct, but a quick grep through the LLVM code base shows that some files in lib/IR use it as well, so LLVMCore probably needs this dependency too.
llvm/utils/TableGen/CMakeLists.txt | ||
---|---|---|
90–91 | Is this right? llvm-tblgen depends on a file generated by llvm-tblgen? |
llvm/utils/TableGen/CMakeLists.txt | ||
---|---|---|
90–91 | Ok, I looked into this further. All the headers that get generated (everything in llvm/include/llvm/) are generated by llvm-min-tblgen, which is defined above. It does make sense for llvm-tblgen to depend on attributes_gen. This is not a concern anymore. |
@bulbazord and I talked offline. It's simpler to keep Attributes.inc under the intrinsics_gen target
Since D74106, target_link_libraries(PRIVATE) brings dependencies on generated headers.
I would like to suggest specifying Core to require usage of intrinsics_gen, unless linking to Core wants to be avoided.
llvm/unittests/Analysis/CMakeLists.txt | ||
---|---|---|
67 ↗ | (On Diff #520493) | This is covered by LINK_COMPONENTS Core |
llvm/utils/TableGen/CMakeLists.txt | ||
90–91 | @jroelofs Could you explain its motivation or background, please? AFAIK this is redundant in normal build. I have confirmed ToT llvm-tblgen doesn't depend on Core or intrinsics_gen. I've noticed it might be required with LLVM_ENABLE_MODULES. Not certain yet though. |
llvm/utils/TableGen/CMakeLists.txt | ||
---|---|---|
90–91 | The change in this file was motivated by the modules build having broken.
The change in the other file was somewhat speculative, based on one of the AnalysisTests sources having included something that intrinsics_gen produces. |
Reverted, as this turned out to cause a cyclic dependency (as @bulbazord suspected). If the Modules bots run into this again, I'm not sure how to fix it...
Re-applying (shortly), because the revert broke the modules build again, which seems to have more short term positive impact than fixing the circular dependency:
While building module 'LLVM_MC' imported from <redacted>/llvm-project/llvm/utils/TableGen/CodeGenRegisters.h:29: While building module 'LLVM_IR' imported from <redacted>/llvm-project/llvm/include/llvm/MC/MCPseudoProbe.h:60: In file included from <module-includes>:1: In file included from <redacted>/llvm-project/llvm/include/llvm/IR/AbstractCallSite.h:18: In file included from <redacted>/llvm-project/llvm/include/llvm/IR/Function.h:25: In file included from <redacted>/llvm-project/llvm/include/llvm/IR/Argument.h:17: <redacted>/llvm-project/llvm/include/llvm/IR/Attributes.h:88:14: fatal error: 'llvm/IR/Attributes.inc' file not found 88 | #include "llvm/IR/Attributes.inc" | ^~~~~~~~~~~~~~~~~~~~~~~~ While building module 'LLVM_MC' imported from <redacted>/llvm-project/llvm/utils/TableGen/CodeGenRegisters.h:29: In file included from <module-includes>:17: In file included from <redacted>/llvm-project/llvm/include/llvm/MC/MCContext.h:22: <redacted>/llvm-project/llvm/include/llvm/MC/MCPseudoProbe.h:60:10: fatal error: could not build module 'LLVM_IR' 60 | #include "llvm/IR/PseudoProbe.h" | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~ In file included from <redacted>/llvm-project/llvm/utils/TableGen/CodeGenTarget.cpp:18: <redacted>/llvm-project/llvm/utils/TableGen/CodeGenRegisters.h:29:10: fatal error: could not build module 'LLVM_MC' 29 | #include "llvm/MC/LaneBitmask.h" | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~ 3 errors generated.
I was able to reproduce @etcwilde's report of the circular dependency (as described in https://reviews.llvm.org/rGd462f65b8242a82d2430605a741825bf10ebaca0) by setting an absolute path: LLVM_TABLEGEN=$BUILD/bin/llvm-tblgen in the CMake config. That pointed me at this, which reveals the circular dependency as it removes the substitution of llvm-min-tblgen for llvm-tblgen for the generated include headers.
commit 95d4506dda79d49e55fdd0e4da7bf81487167aa1 Author: NAKAMURA Takumi <geek4civic@gmail.com> Date: Tue May 2 17:02:06 2023 +0900 add_tablegen: Quick fix to reflect LLVM_TABLEGEN to llvm-min-tblgen `sanitizer-x86_64-linux-android` uses LLVM_TABLEGEN. diff --git a/llvm/cmake/modules/TableGen.cmake b/llvm/cmake/modules/TableGen.cmake index be16127c724e..7fd6628ef55d 100644 --- a/llvm/cmake/modules/TableGen.cmake +++ b/llvm/cmake/modules/TableGen.cmake @@ -154,6 +154,13 @@ macro(add_tablegen target project) endif() endif() + # FIXME: Quick fix to reflect LLVM_TABLEGEN to llvm-min-tblgen + if("${target}" STREQUAL "llvm-min-tblgen" + AND NOT "${LLVM_TABLEGEN}" STREQUAL "" + AND NOT "${LLVM_TABLEGEN}" STREQUAL "llvm-tblgen") + set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}") + endif() + if(ADD_TABLEGEN_EXPORT) set(${project}_TABLEGEN "${${project}_TABLEGEN_DEFAULT}" CACHE STRING "Native TableGen executable. Saves building one when cross-compiling.")
It seems both changes are necessary, but they don't work when we apply them both. @chapuni do you have any advice on how to fix this?
The only reason why you need llvm/IR/Attributes.inc is because you're building the IR module when importing llvm/IR/PseudoProbe.h . It seems to me that llvm/IR/PseudoProbe.h could be moved out of IR and into another library that table gen can actually depend on instead, which seems sensible given that it doesn't depend on anything else in IR and other things in IR don't depend on it.
actually it should probably stay in the IR library as its implementation depends on other IR stuff, but you could split it out of the IR clang module into a separate module since its header is standalone.
I don't know if PseudoProbe belongs to IR (does it contribute to defining LLVM IR?), but the argument seems odd: "everything" depends on IR and that's not a justification to put them there. Maybe I misunderstood your comment?
@jroelofs Sorry for the delay and thanks for the report.
I didn't expect the case of LLVM_TABLEGEN=$BUILD/bin/llvm-tblgen (with path). It would be bad if llvm-min-tblgen is overridden by LLVM_TABLEGEN, that is not an external executable but is built in-tree.
I would like the config not to specify LLVM_TABLEGEN unless it is external. But I could let CMake check whether LLVM_TABLEGEN points actually internal target.
~I think the fact that I've pointed it at an internal target is a red herring. This issue shows up whether it points at an internal one, or an externally-provided one.~
Edit: I was wrong, this circular dependency _only_ shows up when LLVM_TABLEGEN points at the build target.
Is this right? llvm-tblgen depends on a file generated by llvm-tblgen?