This is an archive of the discontinued LLVM Phabricator instance.

Introduce `llvm-min-tblgen` to build public header files
ClosedPublic

Authored by chapuni on Mar 17 2023, 10:24 PM.

Details

Summary

llvm-min-tblgen is capable of building llvm/include/llvm;

  • -gen-attrs
  • -gen-directive-*
  • -gen-intrinsics-*
  • -gen-riscv-target-def
  • -gen-vt

llvm-min-tblgen is built and used only when llvm-tblgen is built in-tree.
This is not installed.

llvm-tblgen is built with complete set and may be installed.
check-llvm uses not llvm-min-tblgen but llvm-tblgen.

LLVM_TABLEGEN_PROJECT overrides the definition of tablegen(project).
LLVM_HEADERS is used as the overridden prefix for LLVM header generators.

If EXPORT is not specified in add_tablegen, its tablegen is treated as internal.

Let llvm-tblgen depend on intrinsics_gen

Depends on D149072

Diff Detail

Event Timeline

chapuni created this revision.Mar 17 2023, 10:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 10:24 PM
chapuni requested review of this revision.Mar 17 2023, 10:24 PM

Changes in test files can be committed separately in advance.

I have just confirmed building. Let me know if there would be issues around installation and customization.

I am planning to move llvm-cg-tblgen to other place, possibly llvm/lib/CodeGen/TableGen.
See also D144351 and https://discourse.llvm.org/t/issues-in-llvm-tblgen-high-parallelized-build/68037/30

mstorsjo added inline comments.Mar 18 2023, 3:22 PM
llvm/utils/TableGen/CMakeLists.txt
68

I think there's a bit of conflict here, when you're calling add_tablegen twice with project set to LLVM.

This parameter is used for setting up the ${project}_TABLEGEN variable, which allows the user to specify a path to an existing such binary when e.g. cross compiling, instead of building a new one - see e.g. https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0/llvm/cmake/modules/TableGen.cmake#L156-L169.

Currently, this can be set via LLVM_TABLEGEN, CLANG_TABLEGEN etc. I'm not sure if we should rename the project parameter there - I think it might work by passing LLVM_CG for that parameter for the new executable, making LLVM_CG_TABLEGEN.

For easier use, one can implicitly set LLVM_NATIVE_TOOL_DIR=<directory> these days, pointing at a directory containing all these *-tblgen binaries. But despite this, the individual setting variables kinda need to work too (and I'm afraid this current form, with clashing options, might cause trouble in build configs where that variable is used, e.g. when cross compiling LLVM).

@mstorsjo How about it?

  • llvm-minimal-tblgen -- Build only in the tree.
  • llvm-tblgen includes codegen emitters. It may be exported. It may be used to check-llvm (my patches to test/TableGen would be unneeded)

@mstorsjo How about it?

  • llvm-minimal-tblgen -- Build only in the tree.
  • llvm-tblgen includes codegen emitters. It may be exported. It may be used to check-llvm (my patches to test/TableGen would be unneeded)

Hmm, unsure... It would hide the fact that there's two separate tools more, making it mostly a concern of the llvm build itself internally, but I wonder how much more complex it would make the cmake logic; it makes more reasoning about the tblgen executables less straightforward...

chapuni updated this revision to Diff 506877.Mar 21 2023, 2:04 AM
chapuni edited the summary of this revision. (Show Details)
  • Adopt D144351. Changes to TableGen.cpp is not needed.
chapuni updated this revision to Diff 507345.Mar 22 2023, 6:43 AM
  • Use llvm-min-tblgen for llvm/include/llvm

@mstorsjo I have introduced llvm-min-tblgen as the special tablegen for headers.
How about it? I think less intrusive.

chapuni retitled this revision from Split out `llvm-cg-tblgen` from `llvm-tblgen` to Introduce `llvm-min-tblgen` to build public header files.Apr 4 2023, 7:08 AM
chapuni edited the summary of this revision. (Show Details)

I have confirmed this works fine for the simple bootstrap.
I expect this wouldn't prevent external llvm-tblgen.

chapuni planned changes to this revision.Apr 11 2023, 6:20 AM

I knew this won't work with build_native_tool.
I supposed I could avoid building host's llvm-min-tblgen.

I will introduce like subproject in add_tablegen.

chapuni updated this revision to Diff 514031.Apr 16 2023, 9:22 AM
  • Restore LLVM_LINK_COMPONENTS
chapuni updated this revision to Diff 514266.Apr 17 2023, 9:06 AM
  • Use llvm-min-tblgen for SupportTests
chapuni updated this revision to Diff 514962.Apr 19 2023, 8:17 AM
  • bazel: s/LLVMTableGenGlobalISel/TableGenGlobalISel/
chapuni updated this revision to Diff 516111.Apr 22 2023, 7:41 PM
chapuni edited the summary of this revision. (Show Details)
  • Rebase
chapuni updated this revision to Diff 516432.Apr 24 2023, 9:08 AM
chapuni edited the summary of this revision. (Show Details)
  • Introduce D149072
  • Use llvm-min-tblgen for SupportTests
chapuni edited the summary of this revision. (Show Details)Apr 24 2023, 9:16 AM
chapuni updated this revision to Diff 516814.Apr 25 2023, 8:14 AM
  • Update comment line

I think this looks quite neat overall, good stuff!

I measured the benefit from this patch in another build case; where I just want to build a small set of executables (for testing compiller-rt) in a new build tree; building tools like FileCheck is quite quick in general, but llvm-config has got surprisingly large numbers of dependencies. With this patch, the number of build steps to build llvm-config drops from 248 to 188, and the total cpu time for compiling it drops from ~11 minutes to ~5 minutes.

llvm/cmake/modules/TableGen.cmake
185 ↗(On Diff #516432)

I guess these two lines could be consider a standalone NFC refactoring/cleanup on top of the current code as well?

chapuni updated this revision to Diff 517191.Apr 26 2023, 8:31 AM
chapuni edited the summary of this revision. (Show Details)
  • Rebase
  • Let llvm-tblgen depend on intrinsics_gen
  • Cleanup llvm/modmap
chapuni updated this revision to Diff 517192.Apr 26 2023, 8:37 AM
  • [Bazel] Let llvm-tblgen depend on :intrinsic_enums_gen

I measured the benefit from this patch in another build case; where I just want to build a small set of executables (for testing compiller-rt) in a new build tree; building tools like FileCheck is quite quick in general, but llvm-config has got surprisingly large numbers of dependencies. With this patch, the number of build steps to build llvm-config drops from 248 to 188, and the total cpu time for compiling it drops from ~11 minutes to ~5 minutes.

I don't guess your background. Does it come from the bottleneck in the full version of llvm-tblgen?

llvm/cmake/modules/TableGen.cmake
185 ↗(On Diff #516432)

It is leftover in my attempts.
Would it be better to rewind it? I prefer this.

I measured the benefit from this patch in another build case; where I just want to build a small set of executables (for testing compiller-rt) in a new build tree; building tools like FileCheck is quite quick in general, but llvm-config has got surprisingly large numbers of dependencies. With this patch, the number of build steps to build llvm-config drops from 248 to 188, and the total cpu time for compiling it drops from ~11 minutes to ~5 minutes.

I don't guess your background. Does it come from the bottleneck in the full version of llvm-tblgen?

Yes; building llvm-config seems to require building some files that currently require the full llvm-tblgen, but after this patch can make do with llvm-min-tblgen.

llvm/cmake/modules/TableGen.cmake
185 ↗(On Diff #516432)

I think the refactoring is good, but I think you could split it out from this patch and apply it directly (or send a separate patch which should be quick and easy to review+approve), and rebase this on top of it.

chapuni updated this revision to Diff 517538.Apr 27 2023, 6:29 AM
chapuni edited the summary of this revision. (Show Details)
  • Restore ${project}-tablegen-host
chapuni marked an inline comment as done.Apr 27 2023, 6:40 AM
chapuni added inline comments.
llvm/cmake/modules/TableGen.cmake
185 ↗(On Diff #516432)

Okay, I've split it to D149343.

chapuni updated this revision to Diff 517741.Apr 27 2023, 4:00 PM
chapuni marked an inline comment as done.
  • Add comment lines

This is the last diff for the series of D148770.

@dblaikie Could you please take a look as a view of layering?

dblaikie accepted this revision.May 1 2023, 4:43 PM

Don't know that I'm a great reviewer for this - I'm really bad at all the build file stuff (but thanks a bunch for having a go at updating the bazel build files - much appreciated). But, at a very rough glance it looks OK to me.

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

I just see this popping up in the recent hours:

[ 20%] Building RISCVTargetParserDef.inc...
llvm-min-tblgen: Unknown command line argument '-gen-riscv-target-def'.  Try: '../../../bin/llvm-min-tblgen --help'
llvm-min-tblgen: Did you mean '--help-list-hidden'?
make[3]: *** [include/llvm/TargetParser/CMakeFiles/RISCVTargetParserTableGen.dir/build.make:118: include/llvm/TargetParser/RISCVTargetParserDef.inc] Error 1
make[2]: *** [CMakeFiles/Makefile2:17904: include/llvm/TargetParser/CMakeFiles/RISCVTargetParserTableGen.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....
[ 20%] Linking CXX static library ../../../../../lib/libMLIRTblgenLib.a

Seems related?

I just see this popping up in the recent hours:

It was an issue in Makefile generator. Resolved in rG73fbe9c9291a.
Sorry for inconvenience.

Besides, I see a few still failing builders, for example
https://lab.llvm.org/buildbot/#/builders/77/builds/26531

I will revert this later if I didn't find any solutions.

I have fixed external LLVM_TABLEGEN in rG95d4506dda79 as a quick fix. I will rewrite it later.