[CMake] -gen-dag-isel: add -omit-comments if neither Debug nor RelWithDebInfo

[MaskRay]

Summary

Omitting comments can make the output much smaller. Size/time impact on
my machine:

• lib/Target/AArch64/AArch64GenDAGISel.inc, 10MiB (8.89s) -> 5MiB (3.20s)
• lib/Target/X86/X86GenDAGISel.inc, 20MiB (6.48s) -> 8.5MiB (4.18s)

In total, this change decreases lib/Target/*/*GenDAGISel.inc from
71.4MiB to 30.1MiB.

[thakis]

Nice find! Should we just disable them always unless someone explicitly asks for them?

Please wait with landing for nhaehnle to chime in.

[MaskRay]

In D78884#2026270, @thakis wrote:

Nice find! Should we just disable them always unless someone explicitly asks for them?

Please wait with landing for nhaehnle to chime in.

Yeah, we can teach more TableGen backends to understand -omit-comments...

I guess comments are occasionally useful for some developers debugging code in generated code. If the build is a non-debug one, then certainly these comments are not needed...

[rnk]

llvm/cmake/modules/TableGen.cmake

59

IMO this condition is a bit awkward, but I can't suggest a better one. I think to be thorough we might want to raise this up to a cmake option next to LLVM_OPTIMIZED_TABLEGEN, but I won't insist on it.

I use a release build, and I appreciate comments in other table generated files, but I have never debugged DAG isel matchers.

[MaskRay]

@nhaehnle :)

[nemanjai]

I am very much opposed to this. I just wish I noticed it before it was approved and pushed. In my opinion, the debug dumps from the SDAG are the most useful and easiest to follow of all of the passes in LLVM. A release + asserts build is a very common way for people to build and without the comments in the .inc files, the debug dumps from the SDAG are useless.

I understand that this reduces the size of the produced code, but I don't think the cost is worth the benefit. Personally, I would not differentiate this from other things we get under #ifndef NDEBUG. If you don't want the comments, build with something that defines NDEBUG.
So I think a much better way to disable comments would be something like:

diff --git a/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp b/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
index d9ec14a..99bc3e9 100644
--- a/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
@@ -36,7 +36,12 @@ cl::OptionCategory DAGISelCat("Options for -gen-dag-isel");
 // To reduce generated source code size.
 static cl::opt<bool> OmitComments("omit-comments",
                                   cl::desc("Do not generate comments"),
-                                  cl::init(false), cl::cat(DAGISelCat));
+#ifndef NDEBUG
+                                  cl::init(false),
+#else
+                                  cl::init(true),
+#endif
+                                  cl::cat(DAGISelCat));
 
 static cl::opt<bool> InstrumentCoverage(
     "instrument-coverage",

[MaskRay]

In D78884#2081075, @nemanjai wrote:

I am very much opposed to this. I just wish I noticed it before it was approved and pushed. In my opinion, the debug dumps from the SDAG are the most useful and easiest to follow of all of the passes in LLVM. A release + asserts build is a very common way for people to build and without the comments in the .inc files, the debug dumps from the SDAG are useless.

I understand that this reduces the size of the produced code, but I don't think the cost is worth the benefit. Personally, I would not differentiate this from other things we get under #ifndef NDEBUG. If you don't want the comments, build with something that defines NDEBUG.
So I think a much better way to disable comments would be something like:

diff --git a/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp b/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
index d9ec14a..99bc3e9 100644
--- a/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
@@ -36,7 +36,12 @@ cl::OptionCategory DAGISelCat("Options for -gen-dag-isel");
 // To reduce generated source code size.
 static cl::opt<bool> OmitComments("omit-comments",
                                   cl::desc("Do not generate comments"),
-                                  cl::init(false), cl::cat(DAGISelCat));
+#ifndef NDEBUG
+                                  cl::init(false),
+#else
+                                  cl::init(true),
+#endif
+                                  cl::cat(DAGISelCat));
 
 static cl::opt<bool> InstrumentCoverage(
     "instrument-coverage",

How are comments helpful if no debugging information is produced (neither RelWithDebInfo nor Debug)? You don't have line tables and the debugger cannot associate the source lines. If you have reasons that -UNDEBUG builds may need comments, we can check LLVM_ENABLE_ASSERTIONS in CMake as well.

[nemanjai]

In D78884#2081132, @MaskRay wrote:

How are comments helpful if no debugging information is produced (neither RelWithDebInfo nor Debug)? You don't have line tables and the debugger cannot associate the source lines. If you have reasons that -UNDEBUG builds may need comments, we can check LLVM_ENABLE_ASSERTIONS in CMake as well.

The dump from -debug-only=isel lists everything the instruction selector tried in order which makes it very easy to reason about what it did. But the indices into the MatcherTable are the comments that this patch removes. The reason I suggested using NDEBUG is that the debug messages from the SDAG that refer to these are also guarded by that macro (since they are LLVM_DEBUG messages).

llvm/cmake/modules/TableGen.cmake

function(tablegen project ofn)
   endif()
   if (LLVM_ENABLE_GISEL_COV)
     list(FIND ARGN "-gen-global-isel" idx)
     if( NOT idx EQUAL -1 )
       list(APPEND LLVM_TABLEGEN_FLAGS "-instrument-gisel-coverage")
       list(APPEND LLVM_TABLEGEN_FLAGS "-gisel-coverage-file=${LLVM_GISEL_COV_PREFIX}all")
     endif()
   endif()
+  # Comments are only useful for Debug builds. Omit them if the backend
+  # supports it.
+  if (NOT (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" OR
+           uppercase_CMAKE_BUILD_TYPE STREQUAL "RELWITHDEBINFO"))
+    list(FIND ARGN "-gen-dag-isel" idx)
+    if (NOT idx EQUAL -1)
+      list(APPEND LLVM_TABLEGEN_FLAGS "-omit-comments")
+    endif()
+  endif()
 
   # MSVC can't support long string literals ("long" > 65534 bytes)[1], so if there's
   # a possibility of generated tables being consumed by MSVC, generate arrays of
   # char literals, instead. If we're cross-compiling, then conservatively assume
   # that the source might be consumed by MSVC.
   # [1] https://docs.microsoft.com/en-us/cpp/cpp/compiler-limits?view=vs-2017
   if (MSVC AND project STREQUAL LLVM)
     list(APPEND LLVM_TABLEGEN_FLAGS "--long-string-literals=0")