[tablegen] Optionally format tablegen targets with clang-format

[dsanders]

Summary

While it's useful to make a reasonable effort to format tablegen output in a
sensible way, it's also easy to get overly bogged down in the formatting
concerns.

This patch adds a way for tablegen-erated intermediate files to opt-in to being
filtered through clang-format when it's available. Files that opt-in to this
will fall back on the output that comes directly from tablegen.

To opt-in, invoke tablegen() using the FORMAT option like so:

tablegen(LLVM AArch64GenRegisterBank.inc FORMAT OPTIONS -gen-register-bank)

[dsanders]

ping

[qcolombet]

Hi Daniel,

That's interesting.
I am a bit concerned about making GISel special in terms of how the output looks. I can see the argument but I am also wondering if we don't what to have a control on that. For instance, in SDISel, each entry in the array is on its own line and clang-format could break that.

Ideally I would like we have the same (post) processing for every tablegen file but given I don't see that happening, (at least in the near future) I would lean toward properly emitting the output from the start.

Cheers,
-Quentin

[dsanders]

My ideal would be version of raw_ostream that allows clang-format style formatting to be switched on and off at will but that doesn't seem to be available and it would require llvm-tblgen to link to (probably old) clang libraries.

For AArch64GenGlobalISel.inc in particular, the main aim is to defer any significant work on pretty-printing until after the content of the file has settled down. The current formatting is difficult to read and debug but fixing it is an intrusive change that's wasted effort given that we intend to replace the bulk of it with an array like SelectionDAG's.

[rovka]

In D30299#710519, @qcolombet wrote:

Hi Daniel,

That's interesting.
I am a bit concerned about making GISel special in terms of how the output looks. I can see the argument but I am also wondering if we don't what to have a control on that. For instance, in SDISel, each entry in the array is on its own line and clang-format could break that.

Is that the case for many of the files that we generate? If not, maybe we can make it opt-out instead?
Alternatively, clang-format accepts some configuration options, is there any combination of those that would make the output of SDISel look ok when clang-formatted?

Ideally I would like we have the same (post) processing for every tablegen file but given I don't see that happening, (at least in the near future) I would lean toward properly emitting the output from the start.

Cheers,
-Quentin

[dsanders]

Sorry for being slow to get back to this one. I've been prioritising the other patches.

I am a bit concerned about making GISel special in terms of how the output looks. I can see the argument but I am also wondering if we don't what to have a control on that. For instance, in SDISel, each entry in the array is on its own line and clang-format could break that.

Is that the case for many of the files that we generate? If not, maybe we can make it opt-out instead?
Alternatively, clang-format accepts some configuration options, is there any combination of those that would make the output of SDISel look ok when clang-formatted?

I think it's beneficial to some of them, for example MatchTable0 in the AsmMatcher is a lot easier to read after clang-format. However there's definitely some things clang-format shouldn't touch. Here's a couple examples:
AArch64GenAsmWriter.inc: AsmStrs isn't particularly readable either way but the comments are difficult to spot after clang-format
AArch64GenDAGISel.inc: Tablegen uses indentation to show the scopes but clang-format destroys this information. GlobalISel will probably end up doing something similar in the long run.

If clang-format has a way to protect certain regions then we could add the appropriate annotations and generally enable it.

CMakeLists.txt

if(CCACHE_PROGRAM)
       set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ${CCACHE_PROGRAM})
   else()
     message(FATAL_ERROR "Unable to find the program ccache. Set LLVM_CCACHE_BUILD to OFF")
   endif()
 endif()
 
 option(LLVM_DEPENDENCY_DEBUGGING "Dependency debugging mode to verify correctly expressed library dependencies (Darwin only)" OFF)
 
+# Some tablegen-erators like to format their output using clang-format when
+# it's available.
+find_program(CLANG_FORMAT_EXECUTABLE clang-format)
+
 # Some features of the LLVM build may be disallowed when dependency debugging is
 # enabled. In particular you cannot use ccache because we want to force compile
 # operations to always happen.
 if(LLVM_DEPENDENCY_DEBUGGING)
   if(NOT CMAKE_HOST_APPLE)
     message(FATAL_ERROR "Dependency debugging is only currently supported on Darwin hosts.")
   endif()
   if(LLVM_CCACHE_BUILD)

cmake/modules/TableGen.cmake

 # LLVM_TARGET_DEFINITIONS must contain the name of the .td file to process.
 # Extra parameters for `tblgen' may come after `ofn' parameter.
 # Adds the name of the generated file to TABLEGEN_OUTPUT.
 
 include(LLVMExternalProjectUtils)
 
 if(LLVM_MAIN_INCLUDE_DIR)
   set(LLVM_TABLEGEN_FLAGS -I ${LLVM_MAIN_INCLUDE_DIR})
 endif()
 
 function(tablegen project ofn)
+  cmake_parse_arguments(tablegen "FORMAT" "" "OPTIONS" ${ARGN})
+  set(tablegen_OPTIONS ${tablegen_OPTIONS} ${tablegen_UNPARSED_ARGUMENTS})
+
   # Validate calling context.
   if(NOT ${project}_TABLEGEN_EXE)
     message(FATAL_ERROR "${project}_TABLEGEN_EXE not set")
   endif()
 
   file(GLOB local_tds "*.td")
   file(GLOB_RECURSE global_tds "${LLVM_MAIN_INCLUDE_DIR}/llvm/*.td")
 
   if (IS_ABSOLUTE ${LLVM_TARGET_DEFINITIONS})
     set(LLVM_TARGET_DEFINITIONS_ABSOLUTE ${LLVM_TARGET_DEFINITIONS})
   else()
     set(LLVM_TARGET_DEFINITIONS_ABSOLUTE
       ${CMAKE_CURRENT_SOURCE_DIR}/${LLVM_TARGET_DEFINITIONS})
   endif()
   if (LLVM_ENABLE_DAGISEL_COV)
-    list(FIND ARGN "-gen-dag-isel" idx)
+    list(FIND tablegen_OPTIONS "-gen-dag-isel" idx)
     if( NOT idx EQUAL -1 )
       list(APPEND LLVM_TABLEGEN_FLAGS "-instrument-coverage")
     endif()
   endif()
 
+  set(tablegen_format_command)
+  if(tablegen_FORMAT)
+    if(CLANG_FORMAT_EXECUTABLE)
+      set(tablegen_format_command COMMAND ${CLANG_FORMAT_EXECUTABLE} -i ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp)
+    else()
+      message(STATUS "Not formatting tablegen output ${ofn} due to lack of clang-format")
+    endif()
+  else()
+  endif()
+
   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp
     # Generate tablegen output in a temporary file.
-    COMMAND ${${project}_TABLEGEN_EXE} ${ARGN} -I ${CMAKE_CURRENT_SOURCE_DIR}
+    COMMAND ${${project}_TABLEGEN_EXE} ${tablegen_OPTIONS} -I ${CMAKE_CURRENT_SOURCE_DIR}
     ${LLVM_TABLEGEN_FLAGS} 
     ${LLVM_TARGET_DEFINITIONS_ABSOLUTE}
     -o ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.tmp
+    # Add a formatting command if we want one.
+    ${tablegen_format_command}
     # The file in LLVM_TARGET_DEFINITIONS may be not in the current
     # directory and local_tds may not contain it, so we must
     # explicitly list it here:
     DEPENDS ${${project}_TABLEGEN_TARGET} ${local_tds} ${global_tds}
     ${LLVM_TARGET_DEFINITIONS_ABSOLUTE}
     COMMENT "Building ${ofn}..."
     )
   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${ofn}

lib/Target/AArch64/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS AArch64.td)
 
 tablegen(LLVM AArch64GenRegisterInfo.inc -gen-register-info)
 tablegen(LLVM AArch64GenInstrInfo.inc -gen-instr-info)
 tablegen(LLVM AArch64GenMCCodeEmitter.inc -gen-emitter)
 tablegen(LLVM AArch64GenMCPseudoLowering.inc -gen-pseudo-lowering)
 tablegen(LLVM AArch64GenAsmWriter.inc -gen-asm-writer)
 tablegen(LLVM AArch64GenAsmWriter1.inc -gen-asm-writer -asmwriternum=1)
 tablegen(LLVM AArch64GenAsmMatcher.inc -gen-asm-matcher)
 tablegen(LLVM AArch64GenDAGISel.inc -gen-dag-isel)
 tablegen(LLVM AArch64GenFastISel.inc -gen-fast-isel)
 tablegen(LLVM AArch64GenCallingConv.inc -gen-callingconv)
 tablegen(LLVM AArch64GenSubtargetInfo.inc -gen-subtarget)
 tablegen(LLVM AArch64GenDisassemblerTables.inc -gen-disassembler)
 tablegen(LLVM AArch64GenSystemOperands.inc -gen-searchable-tables)
 if(LLVM_BUILD_GLOBAL_ISEL)
-  tablegen(LLVM AArch64GenRegisterBank.inc -gen-register-bank)
-  tablegen(LLVM AArch64GenGlobalISel.inc -gen-global-isel)
+  tablegen(LLVM AArch64GenRegisterBank.inc FORMAT OPTIONS -gen-register-bank)
+  tablegen(LLVM AArch64GenGlobalISel.inc FORMAT OPTIONS -gen-global-isel)
 endif()
 
 add_public_tablegen_target(AArch64CommonTableGen)
 
 # List of all GlobalISel files.
 set(GLOBAL_ISEL_FILES
       AArch64CallLowering.cpp
       AArch64InstructionSelector.cpp