cmake: add missing dependency on Attributes.inc

[bulbazord]

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?

[bulbazord]

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.

[jroelofs]

@bulbazord and I talked offline. It's simpler to keep Attributes.inc under the intrinsics_gen target

[bulbazord]

LGTM

[chapuni]

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

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.

[jroelofs]

llvm/utils/TableGen/CMakeLists.txt

90–91

The change in this file was motivated by the modules build having broken.

@bulbazord:
llvm/util/Tablegen/AsmMatcherEmitter.cpp includes llvm/util/Tablegen/CodeGenRegisters.h, which includes some MC header... which includes an IR header, which depends on this .inc file being generated.

The change in the other file was somewhat speculative, based on one of the AnalysisTests sources having included something that intrinsics_gen produces.

[chapuni]

llvm/utils/TableGen/CMakeLists.txt

90–91

Thanks for your explanation. I knew it when I pruned kludges in D146352 (modmap) but I forgot restoring in D148769.
This is better than restoring modmap since it is not a big burden.

[jroelofs]

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...

[jroelofs]

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 <[e-mail]>
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?

[arphaman]

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.

[arphaman]

In D150144#4578352, @arphaman wrote:

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.

[mehdi_amini]

In D150144#4581168, @arphaman wrote:

actually it should probably stay in the IR library as its implementation depends on other IR stuff

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?

[chapuni]

@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.

[jroelofs]

In D150144#4583702, @chapuni wrote:

@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.

llvm/unittests/Analysis/CMakeLists.txt

 else()
   LIST(APPEND LLVM_OPTIONAL_SOURCES ${MLGO_TESTS})
 endif()
 
 add_llvm_unittest_with_input_files(AnalysisTests
   ${ANALYSIS_TEST_SOURCES}
   )
 
+add_dependencies(AnalysisTests intrinsics_gen)
+
 target_link_libraries(AnalysisTests PRIVATE LLVMTestingSupport)
 
 # On AIX, enable run-time linking to allow symbols from the plugins shared
 # objects to be properly bound.
 if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
   set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-brtl")
 endif()
 
 # Export symbols from the plugins shared objects.
 if(NOT WIN32)
   export_executable_symbols_for_plugins(AnalysisTests)
 endif()
 
 add_subdirectory(InlineAdvisorPlugin)
 add_subdirectory(InlineOrderPlugin)

llvm/utils/TableGen/CMakeLists.txt

add_tablegen(llvm-tblgen LLVM
   X86DisassemblerTables.cpp
   X86EVEX2VEXTablesEmitter.cpp
   X86FoldTablesEmitter.cpp
   X86MnemonicTables.cpp
   X86ModRMFilters.cpp
   X86RecognizableInstr.cpp
   WebAssemblyDisassemblerEmitter.cpp
   $<TARGET_OBJECTS:obj.LLVMTableGenCommon>
+
+  DEPENDS
+  intrinsics_gen # via llvm-min-tablegen
   )
 target_link_libraries(llvm-tblgen PRIVATE LLVMTableGenGlobalISel)
 set_target_properties(llvm-tblgen PROPERTIES FOLDER "Tablegenning")