[CMake] Add new cmake option to control adding comments in GenDAGISel

[lei]

Summary

Add new cmake option LLVM_OMIT_DAGISEL_COMMENTS to control adding
of comments in GenDAGISel for none debug builds

Ref: https://reviews.llvm.org/D78884

[MaskRay]

Add a new CMake variable similar to LLVM_OPTIMIZED_TABLEGEN? I think most people don't need comments even for their !NDEBUG builds.

[nemanjai]

In D114122#3139130, @MaskRay wrote:

Add a new CMake variable similar to LLVM_OPTIMIZED_TABLEGEN? I think most people don't need comments even for their !NDEBUG builds.

I always wonder about statements about what "most people need/want". From my experience, most people in my immediate surroundings actually do use debug messages from ISEL and want them to refer to something they can find in .inc files rather than being meaningless. Most of those same people simply don't build debug builds because those are very slow, huge, etc. while just asserts provide enough "debuggability".

While having messages that refer to indices in a file that do not exist seems like simply a bad idea (i.e. why emit meaningless messages at all), I do recognize the fact that comments in these .inc files slow everyone's build and are only useful to back end developers.

All that being said, I am not against adding a CMake variable (perhaps LLVM_OMIT_DAGISEL_COMMENTS) that will be set to true for Release builds and false for Debug/RelWithDebInfo builds.

[lei]

Switch to using new cmake variable to control comments.

[MaskRay]

In D114122#3139717, @nemanjai wrote:

In D114122#3139130, @MaskRay wrote:

Add a new CMake variable similar to LLVM_OPTIMIZED_TABLEGEN? I think most people don't need comments even for their !NDEBUG builds.

I always wonder about statements about what "most people need/want". From my experience, most people in my immediate surroundings actually do use debug messages from ISEL and want them to refer to something they can find in .inc files rather than being meaningless. Most of those same people simply don't build debug builds because those are very slow, huge, etc. while just asserts provide enough "debuggability".

While having messages that refer to indices in a file that do not exist seems like simply a bad idea (i.e. why emit meaningless messages at all), I do recognize the fact that comments in these .inc files slow everyone's build and are only useful to back end developers.

All that being said, I am not against adding a CMake variable (perhaps LLVM_OMIT_DAGISEL_COMMENTS) that will be set to true for Release builds and false for Debug/RelWithDebInfo builds.

I know that you mentioned it. "most people need/want" is anecdotal and from my observation that no other backend developer has complained about it. I could have missed something, though.

To be productive, perhaps we can tag some backend developers. @RKSimon @craig.topper @arsenm @dmgreen

[craig.topper]

In D114122#3141721, @MaskRay wrote:

In D114122#3139717, @nemanjai wrote:

In D114122#3139130, @MaskRay wrote:

Add a new CMake variable similar to LLVM_OPTIMIZED_TABLEGEN? I think most people don't need comments even for their !NDEBUG builds.

I always wonder about statements about what "most people need/want". From my experience, most people in my immediate surroundings actually do use debug messages from ISEL and want them to refer to something they can find in .inc files rather than being meaningless. Most of those same people simply don't build debug builds because those are very slow, huge, etc. while just asserts provide enough "debuggability".

While having messages that refer to indices in a file that do not exist seems like simply a bad idea (i.e. why emit meaningless messages at all), I do recognize the fact that comments in these .inc files slow everyone's build and are only useful to back end developers.

All that being said, I am not against adding a CMake variable (perhaps LLVM_OMIT_DAGISEL_COMMENTS) that will be set to true for Release builds and false for Debug/RelWithDebInfo builds.

I know that you mentioned it. "most people need/want" is anecdotal and from my observation that no other backend developer has complained about it. I could have missed something, though.

To be productive, perhaps we can tag some backend developers. @RKSimon @craig.topper @arsenm @dmgreen

These days I don't find myself using the numbers in those debug message very often. If I do need the comments in the GenDAGISel.inc, I run llvm-tblgen manually to generate a copy with the comments.

[RKSimon]

In D114122#3141755, @craig.topper wrote:

In D114122#3141721, @MaskRay wrote:

In D114122#3139717, @nemanjai wrote:

In D114122#3139130, @MaskRay wrote:

Add a new CMake variable similar to LLVM_OPTIMIZED_TABLEGEN? I think most people don't need comments even for their !NDEBUG builds.

I always wonder about statements about what "most people need/want". From my experience, most people in my immediate surroundings actually do use debug messages from ISEL and want them to refer to something they can find in .inc files rather than being meaningless. Most of those same people simply don't build debug builds because those are very slow, huge, etc. while just asserts provide enough "debuggability".

While having messages that refer to indices in a file that do not exist seems like simply a bad idea (i.e. why emit meaningless messages at all), I do recognize the fact that comments in these .inc files slow everyone's build and are only useful to back end developers.

All that being said, I am not against adding a CMake variable (perhaps LLVM_OMIT_DAGISEL_COMMENTS) that will be set to true for Release builds and false for Debug/RelWithDebInfo builds.

I know that you mentioned it. "most people need/want" is anecdotal and from my observation that no other backend developer has complained about it. I could have missed something, though.

To be productive, perhaps we can tag some backend developers. @RKSimon @craig.topper @arsenm @dmgreen

These days I don't find myself using the numbers in those debug message very often. If I do need the comments in the GenDAGISel.inc, I run llvm-tblgen manually to generate a copy with the comments.

I think I'm one of the few braves souls/idiots who still uses non-optimized tablegen in debug builds when I encounter problems, either approach is fine by me. Having the LLVM_OMIT_DAGISEL_COMMENTS seems 'tidier' in general though.

[nemanjai]

This LGTM but lets give it a few days for others to chime in with their opinion.

[MaskRay]

llvm/CMakeLists.txt

651

[MaskRay]

This patch reverts the patch from

The description needs an update.

[lei]

fix indentation.

llvm/CMakeLists.txt

if (LLVM_LINK_LLVM_DYLIB AND BUILD_SHARED_LIBS)
   message(FATAL_ERROR "Cannot enable BUILD_SHARED_LIBS with LLVM_LINK_LLVM_DYLIB.  We recommend disabling BUILD_SHARED_LIBS.")
 endif()
 
 option(LLVM_OPTIMIZED_TABLEGEN "Force TableGen to be built with optimization" OFF)
 if(CMAKE_CROSSCOMPILING OR (LLVM_OPTIMIZED_TABLEGEN AND (LLVM_ENABLE_ASSERTIONS OR CMAKE_CONFIGURATION_TYPES)))
   set(LLVM_USE_HOST_TOOLS ON)
 endif()
 
+option(LLVM_OMIT_DAGISEL_COMMENTS "Do not add comments to DAG ISel" ON)
+if (CMAKE_BUILD_TYPE AND uppercase_CMAKE_BUILD_TYPE MATCHES "^(RELWITHDEBINFO|DEBUG)$")
+  set(LLVM_OMIT_DAGISEL_COMMENTS OFF)
+endif()
+
 if (MSVC_IDE)
   option(LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION "Configure project to use Visual Studio native visualizers" TRUE)
 endif()
 
 if (LLVM_BUILD_INSTRUMENTED OR LLVM_BUILD_INSTRUMENTED_COVERAGE OR
     LLVM_ENABLE_IR_PGO)
   if(NOT LLVM_PROFILE_MERGE_POOL_SIZE)
     # A pool size of 1-2 is probably sufficient on a SSD. 3-4 should be fine

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"))
+  if (LLVM_OMIT_DAGISEL_COMMENTS)
     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