Place PersistentId declaration under #if LLVM_ENABLE_ABI_BREAKING_CHECKS to
reduce memory usage when it is not needed.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Here is a previous thread on this topic https://lists.llvm.org/pipermail/llvm-dev/2016-July/101842.html
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.)
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. |
@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.
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.
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.
+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!
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.
These should all be #if LLVM_ENABLE_ABI_BREAKING_CHECKS.