[llvm] r349624 - Let TableGen write output only if it changed, instead of doing so in cmake, attempt 2

Nico Weber via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 19 05:35:53 PST 2018


Author: nico
Date: Wed Dec 19 05:35:53 2018
New Revision: 349624

URL: http://llvm.org/viewvc/llvm-project?rev=349624&view=rev
Log:
Let TableGen write output only if it changed, instead of doing so in cmake, attempt 2

This relands r330742:
"""
Let TableGen write output only if it changed, instead of doing so in cmake.

Removes one subprocess and one temp file from the build for each tablegen
invocation.

No intended behavior change.
"""

In particular, if you see rebuilds after this change that you didn't see
before this change, that's unintended and it's fine to revert this change
again (but let me know).

r330742 got reverted because some people reported that llvm-tblgen ran on every
build after it.  This could happen if the depfile output got deleted without
deleting the main .inc output. To fix, make TableGen always write the depfile,
but keep writing the main .inc output only if it has changed. This matches what
we did in cmake before.

Differential Revision: https://reviews.llvm.org/D55842

Modified:
    llvm/trunk/cmake/modules/TableGen.cmake
    llvm/trunk/lib/TableGen/Main.cpp
    llvm/trunk/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni

Modified: llvm/trunk/cmake/modules/TableGen.cmake
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/cmake/modules/TableGen.cmake?rev=349624&r1=349623&r2=349624&view=diff
==============================================================================
--- llvm/trunk/cmake/modules/TableGen.cmake (original)
+++ llvm/trunk/cmake/modules/TableGen.cmake Wed Dec 19 05:35:53 2018
@@ -25,7 +25,7 @@ function(tablegen project ofn)
     file(RELATIVE_PATH ofn_rel
       ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
     set(additional_cmdline
-      -o ${ofn_rel}.tmp
+      -o ${ofn_rel}
       -d ${ofn_rel}.d
       WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
       DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
@@ -36,7 +36,7 @@ function(tablegen project ofn)
     file(GLOB local_tds "*.td")
     file(GLOB_RECURSE global_tds "${LLVM_MAIN_INCLUDE_DIR}/llvm/*.td")
     set(additional_cmdline
-      -o ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp
+      -o ${CMAKE_CURRENT_BINARY_DIR}/${ofn}
       )
   endif()
 
@@ -69,8 +69,7 @@ function(tablegen project ofn)
   # dependency twice in the result file when
   # ("${${project}_TABLEGEN_TARGET}" STREQUAL "${${project}_TABLEGEN_EXE}")
   # but lets us having smaller and cleaner code here.
-  add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp
-    # Generate tablegen output in a temporary file.
+  add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${ofn}
     COMMAND ${${project}_TABLEGEN_EXE} ${ARGN} -I ${CMAKE_CURRENT_SOURCE_DIR}
     ${LLVM_TABLEGEN_FLAGS}
     ${LLVM_TARGET_DEFINITIONS_ABSOLUTE}
@@ -83,20 +82,9 @@ function(tablegen project ofn)
     ${LLVM_TARGET_DEFINITIONS_ABSOLUTE}
     COMMENT "Building ${ofn}..."
     )
-  add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${ofn}
-    # Only update the real output file if there are any differences.
-    # This prevents recompilation of all the files depending on it if there
-    # aren't any.
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp
-        ${CMAKE_CURRENT_BINARY_DIR}/${ofn}
-    DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp
-    COMMENT "Updating ${ofn}..."
-    )
 
   # `make clean' must remove all those generated files:
-  set_property(DIRECTORY APPEND
-    PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${ofn}.tmp ${ofn})
+  set_property(DIRECTORY APPEND PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${ofn})
 
   set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT} ${CMAKE_CURRENT_BINARY_DIR}/${ofn} PARENT_SCOPE)
   set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/${ofn} PROPERTIES

Modified: llvm/trunk/lib/TableGen/Main.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/Main.cpp?rev=349624&r1=349623&r2=349624&view=diff
==============================================================================
--- llvm/trunk/lib/TableGen/Main.cpp (original)
+++ llvm/trunk/lib/TableGen/Main.cpp Wed Dec 19 05:35:53 2018
@@ -100,23 +100,39 @@ int llvm::TableGenMain(char *argv0, Tabl
   if (Parser.ParseFile())
     return 1;
 
-  std::error_code EC;
-  ToolOutputFile Out(OutputFilename, EC, sys::fs::F_Text);
-  if (EC)
-    return reportError(argv0, "error opening " + OutputFilename + ":" +
-                                  EC.message() + "\n");
+  // Write output to memory.
+  std::string OutString;
+  raw_string_ostream Out(OutString);
+  if (MainFn(Out, Records))
+    return 1;
+
+  // Always write the depfile, even if the main output hasn't changed.
+  // If it's missing, Ninja considers the output dirty.  If this was below
+  // the early exit below and someone deleted the .inc.d file but not the .inc
+  // file, tablegen would never write the depfile.
   if (!DependFilename.empty()) {
     if (int Ret = createDependencyFile(Parser, argv0))
       return Ret;
   }
 
-  if (MainFn(Out.os(), Records))
-    return 1;
+  // Only updates the real output file if there are any differences.
+  // This prevents recompilation of all the files depending on it if there
+  // aren't any.
+  if (auto ExistingOrErr = MemoryBuffer::getFile(OutputFilename))
+    if (std::move(ExistingOrErr.get())->getBuffer() == Out.str())
+      return 0;
+
+  std::error_code EC;
+  ToolOutputFile OutFile(OutputFilename, EC, sys::fs::F_Text);
+  if (EC)
+    return reportError(argv0, "error opening " + OutputFilename + ":" +
+                                  EC.message() + "\n");
+  OutFile.os() << Out.str();
 
   if (ErrorsPrinted > 0)
     return reportError(argv0, Twine(ErrorsPrinted) + " errors.\n");
 
   // Declare success.
-  Out.keep();
+  OutFile.keep();
   return 0;
 }

Modified: llvm/trunk/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni?rev=349624&r1=349623&r2=349624&view=diff
==============================================================================
--- llvm/trunk/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni (original)
+++ llvm/trunk/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni Wed Dec 19 05:35:53 2018
@@ -60,11 +60,6 @@ template("tablegen") {
     depfile = "$gen_output.d"
     td_file = rebase_path(td_file, root_build_dir)
 
-    # FIXME: The cmake build lets tablegen write to a temp file and then copies
-    # it over the final output only if it has changed, for ninja's restat
-    # optimization. Instead of doing that in cmake, llvm-tblgen should do this
-    # itself. r330742 tried this, but it caused problems. Fix those and reland,
-    # so that the gn build has the optimization too.
     args = [
              rebase_path(tblgen_executable, root_build_dir),
              "-I",




More information about the llvm-commits mailing list