[llvm-(min-)tblgen] Avoid redundant source compilation #114494

Meinersbur · 2024-11-01T00:57:36Z

All the sources of llvm-min-tblgen are also used for llvm-tblgen, with identical compilation flags. Reuse the object files of llvm-min-tblgen for llvm-tblgen by applying the usual source structure of an executable: One file per executable which named after the executable name containing the (in this case trivial) main function, which just calls the tblgen_main in TableGen.cpp. This should also clear up any confusion (including mine) of where each executable's main function is.

While this slightly reduces build time, the main motivation is ccache. Using the hard_link option, building the object files for llvm-tblgen will result in a hard link to the same object file already used for llvm-min-tblgen. To signal the build system that the file is new, ccache will update the file's time stamp. Unfortunately, time stamps are shared between all hard-linked files s.t. this will indirectly also update the time stamps for the object files used for llvm-tblgen. At the next run, Ninja will recognize this time stamp discrepancy to the expected stamp recorded in .ninja_log and rebuild those object files for llvm-min-tblgen, which again will also update the stamp for the llvm-tblgen... . This is especially annoying for tablegen because it means Ninja will re-run all tablegenning in every build.

I am using the hard_link option because it reduces the cost of having multiple build-trees of the LLVM sources and reduces the wear to the SSD they are stored on.

Meinersbur · 2024-11-01T12:08:00Z

Build failure is unrelated: lldb-api :: tools/lldb-dap/evaluate/TestDAP_evaluate.py

llvmbot · 2024-11-01T12:09:10Z

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-tablegen

Author: Michael Kruse (Meinersbur)

Changes

All the sources of llvm-min-tblgen are also used for llvm-tblgen, with identical compilation flags. Reuse the object files of llvm-min-tblgen for llvm-tblgen.

While this slightly reduces build time, the main motivation is ccache. Using the hard_link option, building the object files for llvm-tblgen will result in a hard link to the same object file already used for llvm-min-tblgen. To signal the build system that the file is new, ccache will update the file's time stamp. Unfortunately, time stamps a shared between all hard-linked file s.t. this will also update the time stamps for the object files used for llvm-min-tblgen. At the next run, Ninja will recognize this time stamp discrepancy to the expected stamp recorded in .ninja_log and rebuild those object files for llvm-min-tblgen, which again will also update the stamp for the llvm-min-tblgen... . This is especially annoying for llvm-(min)-tablegen because it means Ninja will rerun all tablegenning in every build.

I am using the hard_link option because it reduces the cost of having multiple build-trees of the same LLVM source tree and reduces the wear to the SSD they are stored on.

Full diff: https://github.com/llvm/llvm-project/pull/114494.diff

1 Files Affected:

(modified) llvm/utils/TableGen/CMakeLists.txt (+2-7)

diff --git a/llvm/utils/TableGen/CMakeLists.txt b/llvm/utils/TableGen/CMakeLists.txt
index ba1e4aa01b48d6..ded2e228dcd6de 100644
--- a/llvm/utils/TableGen/CMakeLists.txt
+++ b/llvm/utils/TableGen/CMakeLists.txt
@@ -32,10 +32,8 @@ set(LLVM_LINK_COMPONENTS
 add_tablegen(llvm-tblgen LLVM
   DESTINATION "${LLVM_TOOLS_INSTALL_DIR}"
   EXPORT LLVM
-  ARMTargetDefEmitter.cpp
   AsmMatcherEmitter.cpp
   AsmWriterEmitter.cpp
-  Attributes.cpp
   CallingConvEmitter.cpp
   CodeEmitterGen.cpp
   CodeGenMapTable.cpp
@@ -48,7 +46,6 @@ add_tablegen(llvm-tblgen LLVM
   DecoderEmitter.cpp
   DFAEmitter.cpp
   DFAPacketizerEmitter.cpp
-  DirectiveEmitter.cpp
   DisassemblerEmitter.cpp
   DXILEmitter.cpp
   ExegesisEmitter.cpp
@@ -57,18 +54,14 @@ add_tablegen(llvm-tblgen LLVM
   GlobalISelEmitter.cpp
   InstrDocsEmitter.cpp
   InstrInfoEmitter.cpp
-  IntrinsicEmitter.cpp
   MacroFusionPredicatorEmitter.cpp
   OptionParserEmitter.cpp
   OptionRSTEmitter.cpp
   PseudoLoweringEmitter.cpp
   RegisterBankEmitter.cpp
   RegisterInfoEmitter.cpp
-  RISCVTargetDefEmitter.cpp
   SearchableTableEmitter.cpp
   SubtargetEmitter.cpp
-  TableGen.cpp
-  VTEmitter.cpp
   WebAssemblyDisassemblerEmitter.cpp
   X86InstrMappingEmitter.cpp
   X86DisassemblerTables.cpp
@@ -76,6 +69,8 @@ add_tablegen(llvm-tblgen LLVM
   X86MnemonicTables.cpp
   X86ModRMFilters.cpp
   X86RecognizableInstr.cpp
+
+  $<TARGET_OBJECTS:llvm-min-tblgen>
   $<TARGET_OBJECTS:obj.LLVMTableGenBasic>
   $<TARGET_OBJECTS:obj.LLVMTableGenCommon>

chapuni

Thanks for the information.

Looks like #80847 rewound my change.
@Pierre-vh Sorry I was not aware of changes in my review.

chapuni · 2024-11-01T12:33:46Z

llvm/utils/TableGen/CMakeLists.txt

  WebAssemblyDisassemblerEmitter.cpp
  X86InstrMappingEmitter.cpp
  X86DisassemblerTables.cpp
  X86FoldTablesEmitter.cpp
  X86MnemonicTables.cpp
  X86ModRMFilters.cpp
  X86RecognizableInstr.cpp
+
+  $<TARGET_OBJECTS:llvm-min-tblgen>


It wouldn't work. Look into 137d803.
Restoring part of 137d803 may be an option.

In a35bd89 I created another OBJECT library. Do you think it should be merge into TableGenBasic?

If the latter, should the "both" files moved into the Basic/ subdir?

This reverts commit a35bd89.

github-actions · 2024-11-04T16:42:39Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff dd30aa83aa12e5b2b5e58cb72ec85070f725df34 a1849e68b304738573ffb5f828a8e1aa121596e4 --extensions h,cpp -- llvm/utils/TableGen/Basic/TableGen.h llvm/utils/TableGen/llvm-min-tblgen.cpp llvm/utils/TableGen/llvm-tblgen.cpp llvm/utils/TableGen/Basic/ARMTargetDefEmitter.cpp llvm/utils/TableGen/Basic/Attributes.cpp llvm/utils/TableGen/Basic/DirectiveEmitter.cpp llvm/utils/TableGen/Basic/IntrinsicEmitter.cpp llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp llvm/utils/TableGen/Basic/TableGen.cpp llvm/utils/TableGen/Basic/VTEmitter.cpp

View the diff from clang-format here.

diff --git a/llvm/utils/TableGen/Basic/ARMTargetDefEmitter.cpp b/llvm/utils/TableGen/Basic/ARMTargetDefEmitter.cpp
index 3b02f63e94..d4aa1bcf4e 100644
--- a/llvm/utils/TableGen/Basic/ARMTargetDefEmitter.cpp
+++ b/llvm/utils/TableGen/Basic/ARMTargetDefEmitter.cpp
@@ -275,14 +275,15 @@ static void emitARMTargetDef(const RecordKeeper &RK, raw_ostream &OS) {
     auto Name = Rec->getValueAsString("Name");
     auto Alias = Rec->getValueAsString("Alias");
     if (!Processors.contains(Alias))
-      PrintFatalError(
-          Rec, "Alias '" + Name + "' references a non-existent ProcessorModel '" + Alias + "'");
+      PrintFatalError(Rec, "Alias '" + Name +
+                               "' references a non-existent ProcessorModel '" +
+                               Alias + "'");
     if (Processors.contains(Name))
-      PrintFatalError(
-          Rec, "Alias '" + Name + "' duplicates an existing ProcessorModel");
+      PrintFatalError(Rec, "Alias '" + Name +
+                               "' duplicates an existing ProcessorModel");
     if (!Aliases.insert(Name).second)
-      PrintFatalError(
-          Rec, "Alias '" + Name + "' duplicates an existing ProcessorAlias");
+      PrintFatalError(Rec, "Alias '" + Name +
+                               "' duplicates an existing ProcessorAlias");
 
     OS << llvm::formatv(R"(  { "{0}", "{1}" },)", Name, Alias) << '\n';
   }
diff --git a/llvm/utils/TableGen/Basic/VTEmitter.cpp b/llvm/utils/TableGen/Basic/VTEmitter.cpp
index d02932dd5e..3b0809441a 100644
--- a/llvm/utils/TableGen/Basic/VTEmitter.cpp
+++ b/llvm/utils/TableGen/Basic/VTEmitter.cpp
@@ -131,7 +131,7 @@ void VTEmitter::run(raw_ostream &OS) {
     bool IsScalable = VT->getValueAsBit("isScalable");
     bool IsRISCVVecTuple = VT->getValueAsBit("isRISCVVecTuple");
     int64_t NF = VT->getValueAsInt("NF");
-    bool IsNormalValueType =  VT->getValueAsBit("isNormalValueType");
+    bool IsNormalValueType = VT->getValueAsBit("isNormalValueType");
     int64_t NElem = IsVector ? VT->getValueAsInt("nElem") : 0;
     StringRef EltName = IsVector ? VT->getValueAsDef("ElementType")->getName()
                                  : "INVALID_SIMPLE_VALUE_TYPE";

Meinersbur · 2024-12-06T19:23:40Z

@chapuni ping

chapuni

Almost good. Please make sure merging onto main before committing.

chapuni · 2024-12-14T08:07:45Z

llvm/utils/TableGen/Basic/CMakeLists.txt

  SDNodeProperties.cpp
+  TableGen.cpp


I suggest TableGen.cpp may be individual. IIRC, some cmake generators don't like add_executable w/o any source files.

CMake has a problem to identify which linker to use (C or C++) when there is no .c/.cpp file in the list. A strategy used elsewhere also in LLVM adds an empty dummy.cpp into the list.

Keeping TableGen.cpp (or using the same dummy.cpp) in both executables would defeat the entire purpose of this patch: The same timestamp issue still occurs with even just a single file which causes all .td files to be processed again in a no-change build (see summary).

I instead applied the usual source structure of an executable: One file per executable which named after the executable name containing the (in this case trivial) main function, which just calls the tblgen_main in TableGen.cpp. This should also clear up any confusion (including mine) of where each executable's main function is.

chapuni · 2024-12-16T10:42:20Z

llvm/utils/TableGen/CMakeLists.txt

-  IntrinsicEmitter.cpp
-  RISCVTargetDefEmitter.cpp
-  VTEmitter.cpp
+  llvm-min-tblgen.cpp


Meant, one TableGen.cpp for multiple instances. I don't think we need to create copied files.

PARTIAL_SOURCES_INTENDED will accept duplicates.

Please read #114494 (comment) for why not. The duplicate is the exactly the problem this PR intends to solve, even if PARTIAL_SOURCES_INTENDED accepts it.

chapuni · 2024-12-16T10:44:52Z

llvm/utils/TableGen/CMakeLists.txt

@@ -79,6 +70,8 @@ add_tablegen(llvm-tblgen LLVM
  $<TARGET_OBJECTS:obj.LLVMTableGenBasic>
  $<TARGET_OBJECTS:obj.LLVMTableGenCommon>

+  PARTIAL_SOURCES_INTENDED


It may be pruned.

Meinersbur · 2024-12-16T10:47:10Z

Almost good. Please make sure merging onto main before committing.

Any change in a moved/renamed file shows up in a conflict in GitHub, even if nothing in the moved file was changed, so a new conflict showing up between push and reviews is very likely. When merging main, git recogizes the renamed and automatically applies any upstream changes to the new location. GitHub's auto-merge UI does not.

This is also the reason why the code_formatter bot does not pass: it wants to reformat the moved files entirely. If we want them to be formatted, it should be done in a separate "format code" patch. Doing it here would cause actual merge conflicts with the formatted/moved and unformated/original files.

Meinersbur · 2024-12-18T13:04:25Z

@chapuni You already LGTM'ed this PR and did not revoke it after the edit, so I am assuming you are still OK with it (there are no copied files, but two new boilerplate ones that in the future may diverge when adding new features to only one of them). So I will push this at the end of this week. It would still be good if you explicitly OK's this change.

chapuni · 2024-12-20T11:02:02Z

I don't have strong opinions of this. I misunderstood this as reduction of duplications.

I think this is the issue in ccache. That said, I don't think I should stop this.
Please wait for other opinions.

Meinersbur · 2024-12-20T14:17:05Z

I think this is the issue in ccache.

Independent of whether there is an issue with ccache, compiling any file twice is redundant work that should not happen in a well-designed build system.

llvm-ci · 2025-01-02T22:24:37Z

LLVM Buildbot has detected a new failure on builder flang-aarch64-libcxx running on linaro-flang-aarch64-libcxx while building llvm at step 5 "build-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/13541