Page MenuHomePhabricator

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

Authored by lei on Wed, Nov 17, 2:03 PM.

Details

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

Diff Detail

Event Timeline

lei created this revision.Wed, Nov 17, 2:03 PM
lei requested review of this revision.Wed, Nov 17, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 17, 2:03 PM
lei added a reviewer: Restricted Project.Wed, Nov 17, 2:05 PM
MaskRay added a comment.EditedWed, Nov 17, 6:36 PM

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

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 updated this revision to Diff 388342.Thu, Nov 18, 3:48 PM

Switch to using new cmake variable to control comments.

lei retitled this revision from [CMake] Use NDEBUG macro to control adding comments in GenDAGISel.inc to [CMake] Add LLVM_OMIT_DAGISEL_COMMENTS to control adding comments in GenDAGISel.Thu, Nov 18, 3:56 PM
lei retitled this revision from [CMake] Add LLVM_OMIT_DAGISEL_COMMENTS to control adding comments in GenDAGISel to [CMake] Add new cmake option to control adding comments in GenDAGISel.

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

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.

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 accepted this revision.Mon, Nov 22, 5:46 AM

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

This revision is now accepted and ready to land.Mon, Nov 22, 5:46 AM
MaskRay accepted this revision.Mon, Nov 22, 3:20 PM
MaskRay added inline comments.
llvm/CMakeLists.txt
651

This patch reverts the patch from

The description needs an update.

lei edited the summary of this revision. (Show Details)Thu, Nov 25, 6:25 AM
lei updated this revision to Diff 389813.Thu, Nov 25, 8:37 AM
lei edited the summary of this revision. (Show Details)

fix indentation.