Page MenuHomePhabricator

[CodeGen] Place SDNode debug ID declaration under appropriate #if
ClosedPublic

Authored by kovdan01 on Mar 1 2022, 4:00 AM.

Details

Summary

Place PersistentId declaration under #if LLVM_ENABLE_ABI_BREAKING_CHECKS to
reduce memory usage when it is not needed.

Diff Detail

Event Timeline

kovdan01 requested review of this revision.Mar 1 2022, 4:00 AM
kovdan01 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 4:00 AM
lattner resigned from this revision.Mar 1 2022, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 12:24 PM

Thanks! Inspected that, and found an interesting patchset http://www.nuanti.com/tmp/llvm-api-stability/ in discussion https://lists.llvm.org/pipermail/llvm-dev/2013-November/067463.html. And that was not the only attempt to introduce LLVM_NDEBUG - one more was https://reviews.llvm.org/D11833.

In the last review that I mentioned @dexonsmith said that we can use LLVM_ENABLE_ABI_BREAKING_CHECKS instead of NDEBUG in headers. In context of the current patch that will look unsuitable IMHO - PersistentId is only used for debug printing, and a person might want to have PersistentId defined in debug build with ABI breaking checks disabled.

As for me, looks like we can try to introduce LLVM_NDEBUG one more time :) @craig.topper, @dexonsmith, what do you think about it?

Thanks! Inspected that, and found an interesting patchset http://www.nuanti.com/tmp/llvm-api-stability/ in discussion https://lists.llvm.org/pipermail/llvm-dev/2013-November/067463.html. And that was not the only attempt to introduce LLVM_NDEBUG - one more was https://reviews.llvm.org/D11833.

In the last review that I mentioned @dexonsmith said that we can use LLVM_ENABLE_ABI_BREAKING_CHECKS instead of NDEBUG in headers. In context of the current patch that will look unsuitable IMHO - PersistentId is only used for debug printing, and a person might want to have PersistentId defined in debug build with ABI breaking checks disabled.

As for me, looks like we can try to introduce LLVM_NDEBUG one more time :) @craig.topper, @dexonsmith, what do you think about it?

LLVM_ENABLE_ABI_BREAKING_CHECKS is the right thing to do here. This is exactly the case it exists for. If people don't want ABI-breaking things in their release builds, they won't (or shouldn't) turn on LLVM_ENABLE_ABI_BREAKING_CHECKS.

Re-reading your comment:

and a person might want to have PersistentId defined in debug build with ABI breaking checks disabled.

I think that person won't get what they want. There are basically two ABIs:

  • LLVM_ENABLE_ABI_BREAKING_CHECKS == 1: ABI designed for builds that have assertions on.
  • LLVM_ENABLE_ABI_BREAKING_CHECKS == 0: ABI designed for builds that have assertions off.

The purpose of LLVM_ENABLE_ABI_BREAKING_CHECKS is to allow assertions-enabled code to link against assertions-disabled code, as long as they have the same value for LLVM_ENABLE_ABI_BREAKING_CHECKS. That comes with caveats:

  • A release build with LLVM_ENABLE_ABI_BREAKING_CHECKS == 1 will have a bloated ABI.
  • An asserts build with LLVM_ENABLE_ABI_BREAKING_CHECKS == 0 will miss some debugging / checks.

But it's better than not supporting cross-linking at all.

(I don't have an opinion about whether there should be an LLVM_NDEBUG flag that was used pervasively instead of NDEBUG, but I think it's irrelevant for this patch. Even if we had LLVM_NDEBUG, then LLVM_ENABLE_ABI_BREAKING_CHECKS would still be the right thing to use here.)

dexonsmith requested changes to this revision.Mar 11 2022, 11:54 AM
dexonsmith added a subscriber: bogner.

You'll also need to update uses, such as:

void SelectionDAG::InsertNode(SDNode *N) {
  AllNodes.push_back(N);
#ifndef NDEBUG
  N->PersistentId = NextPersistentId++;
  VerifySDNode(N);
#endif
  for (DAGUpdateListener *DUL = UpdateListeners; DUL; DUL = DUL->Next)
    DUL->NodeInserted(N);
}

Not sure what we want here. Could either of:

#ifndef NDEBUG
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
  N->PersistentId = NextPersistentId++;
#endif
  VerifySDNode(N);
#endif

or:

#if LLVM_ENABLE_ABI_BREAKING_CHECKS
  N->PersistentId = NextPersistentId++;
#endif
#ifndef NDEBUG
  VerifySDNode(N);
#endif

Depending on whether maintaining these persistent IDs is expensive. My intuition would be to do the latter; I doubt they're expensive to maintain, so if they're in the ABI we might as well take advantage of them for better debugging. Someone more familiar with SelectionDAG can make the call though.

Also, I don't have an informed opinion on whether this is a good thing to do. It could be that debugging release builds of SelectionDAG with persistent IDs is so valuable that it's worth the memory overhead. Hopefully someone else can chime in on that and the previous question (maybe @craig.topper or @bogner have opinions).

llvm/include/llvm/CodeGen/SelectionDAG.h
283

These should all be #if LLVM_ENABLE_ABI_BREAKING_CHECKS.

This revision now requires changes to proceed.Mar 11 2022, 11:54 AM
kovdan01 updated this revision to Diff 414926.Mar 13 2022, 4:55 AM
kovdan01 retitled this revision from [CodeGen] Place SDNode debug ID declaration under ifndef to [CodeGen] Use SDNode debug ID declaration in release builds.
kovdan01 edited the summary of this revision. (Show Details)

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

kovdan01 updated this revision to Diff 415786.Mar 16 2022, 5:13 AM

Fix formatting according to pre-merge checks. @dexonsmith Please let me know if you have some comments on the changes. I added more info in my previous comment. Thanks!

@dexonsmith Previously you requested changes to this revision. Please let me know if you still see some problems - updated the revision, please see my last comments for details. Thanks!

Thanks for the pings! I'd missed your updates.

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

Which tests are you talking about? Can those tests be marked as UNSUPPORTED when ABI-breaking checks are off?

If not, why not?

It would be good to add a comment to the sources explaining why LLVM_ENABLE_ABI_BREAKING_CHECKS can't/shouldn't be used for PersistentId, and potentially a FIXME with how to fix the blockers such as they are. Probably next to the declaration of the field.

kovdan01 updated this revision to Diff 419195.Mar 30 2022, 10:07 AM
kovdan01 edited the summary of this revision. (Show Details)

[CodeGen] Place SDNode debug ID declaration under appropriate #if

@dexonsmith

Thanks for the pings! I'd missed your updates.

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

Which tests are you talking about? Can those tests be marked as UNSUPPORTED when ABI-breaking checks are off?

If not, why not?

It would be good to add a comment to the sources explaining why LLVM_ENABLE_ABI_BREAKING_CHECKS can't/shouldn't be used for PersistentId, and potentially a FIXME with how to fix the blockers such as they are. Probably next to the declaration of the field.

Currently there is no check for ABI-breaking changes (see llvm/test/lit.site.cfg.py.in). Of course, that could be easily added, but I decided to update the tests themselves to make them work with SDNode debug IDs both in t<number> and 0x<hex address> formats. Also changes update_llc_test_checks.py, and it looks like that we can support more test scenarios for isel in it (not only resulting selection DAG, but also initial and intermediate ones, instcombine phase, etc.)

@dexonsmith

Thanks for the pings! I'd missed your updates.

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

Which tests are you talking about? Can those tests be marked as UNSUPPORTED when ABI-breaking checks are off?

If not, why not?

It would be good to add a comment to the sources explaining why LLVM_ENABLE_ABI_BREAKING_CHECKS can't/shouldn't be used for PersistentId, and potentially a FIXME with how to fix the blockers such as they are. Probably next to the declaration of the field.

Currently there is no check for ABI-breaking changes (see llvm/test/lit.site.cfg.py.in). Of course, that could be easily added, but I decided to update the tests themselves to make them work with SDNode debug IDs both in t<number> and 0x<hex address> formats. Also changes update_llc_test_checks.py, and it looks like that we can support more test scenarios for isel in it (not only resulting selection DAG, but also initial and intermediate ones, instcombine phase, etc.)

Hmm, the tests look much harder to read now. I'm not sure that's better.

Seems simpler/cleaner to use REQUIRES: asserts, abi-breaking-checks.

kovdan01 updated this revision to Diff 419640.Mar 31 2022, 11:47 PM
kovdan01 retitled this revision from [CodeGen] Use SDNode debug ID declaration in release builds to [CodeGen] Place SDNode debug ID declaration under appropriate #if.
kovdan01 edited the summary of this revision. (Show Details)

Hmm, the tests look much harder to read now. I'm not sure that's better.

Seems simpler/cleaner to use REQUIRES: asserts, abi-breaking-checks.

OK, makes sense. Updated the patch following your advice.

kovdan01 updated this revision to Diff 419680.Apr 1 2022, 2:25 AM

Fix broken tests for targets not covered by the patch previously

@dexonsmith Please let me know if you see some issues in the patch. Thanks!

dexonsmith accepted this revision.Apr 5 2022, 8:10 PM

@dexonsmith Please let me know if you see some issues in the patch. Thanks!

LGTM! Thanks for this.

This revision is now accepted and ready to land.Apr 5 2022, 8:10 PM
This revision was landed with ongoing or failed builds.Apr 6 2022, 4:17 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 6 2022, 5:28 AM

+1 to craig.topper's point. It's true that this is what ABI_BREAKING_CHECKS is for, but we should only use this if it saves significant memory. This uint16_t lives next to two bools, so removing it doesn't save any memory (it'll just become padding) as far as I can tell.

Have you seen an actual memory use improvement from this?

As far as I can tell, this adds a bunch of complexity without a benefit (?)

kovdan01 added a comment.EditedApr 6 2022, 6:21 AM

+1 to craig.topper's point. It's true that this is what ABI_BREAKING_CHECKS is for, but we should only use this if it saves significant memory. This uint16_t lives next to two bools, so removing it doesn't save any memory (it'll just become padding) as far as I can tell.

Have you seen an actual memory use improvement from this?

As far as I can tell, this adds a bunch of complexity without a benefit (?)

Hmm, I missed the fact that removing PersistentId will not save memory because of alignment and padding. Initially the motivation was more about stylish improvement – not having fields that we do not need, and then I just extrapolated that to possible memory usage enhancement without keeping padding in mind.

Experimented with that stuff, and I actually do not see memory usage decrease, you are correct: sizeof(SDNode) remains 80 on x86-64 linux. Regarding SelectionDAG – its layout, I suppose, could be changed somehow to save memory, but that will not give noticeable improvements because we do not have many SelectionDAG objects.

I suppose that the patch might become useful in terms of memory usage enhancement in future (if SDNode’s layout changes somehow), but as for now that’s more about codestyle.

If your usecase requires having PersistentId declared and kept correct in all build types and/or complexity looks inapplicable for you, I can revert the patch and submit a new one adding a commect why we do not place these fields under defines not to confure anyone more. Please let me know if reverting the patch is crucial for you. Thanks!

thakis added a comment.Apr 6 2022, 9:35 AM

I think we should revert this then. It makes a whole bunch of tests not run by default when abi_breaking_checks are off, and it kind of needlessly adds abi_breaking_checks as a lit feature. Having fewer lit features is desirable, and if this doesn't have a measurable benefit I don't think we should take on that cost.