This is an archive of the discontinued LLVM Phabricator instance.

cmake: add missing dependency on Attributes.inc
ClosedPublic

Authored by jroelofs on May 8 2023, 2:14 PM.

Diff Detail

Event Timeline

jroelofs created this revision.May 8 2023, 2:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 2:14 PM
jroelofs requested review of this revision.May 8 2023, 2:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 2:14 PM
jroelofs updated this revision to Diff 520483.May 8 2023, 2:29 PM
jroelofs added reviewers: mehdi_amini, jrtc27.
bulbazord requested changes to this revision.May 8 2023, 2:42 PM

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?

This revision now requires changes to proceed.May 8 2023, 2:42 PM
bulbazord added inline comments.May 8 2023, 3:12 PM
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 updated this revision to Diff 520488.May 8 2023, 3:15 PM
jroelofs updated this revision to Diff 520490.May 8 2023, 3:31 PM

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

This revision is now accepted and ready to land.May 8 2023, 3:33 PM
This revision was landed with ongoing or failed builds.May 8 2023, 3:36 PM
This revision was automatically updated to reflect the committed changes.

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.

jroelofs added inline comments.May 9 2023, 8:14 AM
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 added inline comments.May 9 2023, 2:17 PM
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.

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.

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.

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?

@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 added a comment.EditedAug 14 2023, 8:17 AM

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