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

[kovdan01]

Summary

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

[craig.topper]

Here is a previous thread on this topic https://lists.llvm.org/pipermail/llvm-dev/2016-July/101842.html

[kovdan01]

In D120714#3353070, @craig.topper wrote:

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?

[dexonsmith]

In D120714#3375195, @kovdan01 wrote:

In D120714#3353070, @craig.topper wrote:

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.

[dexonsmith]

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]

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

279 ↗

(On Diff #412039)

These should all be #if LLVM_ENABLE_ABI_BREAKING_CHECKS.

[kovdan01]

@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]

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!

[kovdan01]

@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!

[dexonsmith]

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

In D120714#3377826, @kovdan01 wrote:

@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]

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

[kovdan01]

@dexonsmith

In D120714#3400246, @dexonsmith wrote:

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

In D120714#3377826, @kovdan01 wrote:

@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]

In D120714#3417193, @kovdan01 wrote:

@dexonsmith

In D120714#3400246, @dexonsmith wrote:

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

In D120714#3377826, @kovdan01 wrote:

@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]

In D120714#3417416, @dexonsmith wrote:

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]

Fix broken tests for targets not covered by the patch previously

[kovdan01]

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

[dexonsmith]

In D120714#3429834, @kovdan01 wrote:

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

LGTM! Thanks for this.

[thakis]

+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]

In D120714#3432615, @thakis wrote:

+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]

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.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 #endif // NDEBUG
 
 /// Insert a newly allocated node into the DAG.
 ///
 /// Handles insertion into the all nodes list and CSE map, as well as
 /// verification and other common operations when a new node is allocated.
 void SelectionDAG::InsertNode(SDNode *N) {
   AllNodes.push_back(N);
-#ifndef NDEBUG
   N->PersistentId = NextPersistentId++;
+#ifndef NDEBUG
   VerifySDNode(N);
 #endif
   for (DAGUpdateListener *DUL = UpdateListeners; DUL; DUL = DUL->Next)
     DUL->NodeInserted(N);
 }
 
 /// RemoveNodeFromCSEMaps - Take the specified node out of the CSE map that
 /// correspond to it.  This is useful when we're about to delete or repurpose
return MF->getFunction().hasOptSize() ||
       llvm::shouldOptimizeForSize(FLI->MBB->getBasicBlock(), PSI, BFI);
 }
 
 void SelectionDAG::allnodes_clear() {
   assert(&*AllNodes.begin() == &EntryNode);
   AllNodes.remove(AllNodes.begin());
   while (!AllNodes.empty())
     DeallocateNode(&AllNodes.front());
-#ifndef NDEBUG
   NextPersistentId = 0;
-#endif
 }
 
 SDNode *SelectionDAG::FindNodeOrInsertPos(const FoldingSetNodeID &ID,
                                           void *&InsertPos) {
   SDNode *N = CSEMap.FindNodeOrInsertPos(ID, InsertPos);
   if (N) {
     switch (N->getOpcode()) {
     default: break;

llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp

const char *SDNode::getIndexedModeName(ISD::MemIndexedMode AM) {
   case ISD::PRE_INC:    return "<pre-inc>";
   case ISD::PRE_DEC:    return "<pre-dec>";
   case ISD::POST_INC:   return "<post-inc>";
   case ISD::POST_DEC:   return "<post-dec>";
   }
 }
 
 static Printable PrintNodeId(const SDNode &Node) {
-  return Printable([&Node](raw_ostream &OS) {
-#ifndef NDEBUG
-    OS << 't' << Node.PersistentId;
-#else
-    OS << (const void*)&Node;
-#endif
-  });
+  return Printable(
+      [&Node](raw_ostream &OS) { OS << 't' << Node.PersistentId; });
 }
 
 // Print the MMO with more information from the SelectionDAG.
 static void printMemOperand(raw_ostream &OS, const MachineMemOperand &MMO,
                             const MachineFunction *MF, const Module *M,
                             const MachineFrameInfo *MFI,
                             const TargetInstrInfo *TII, LLVMContext &Ctx) {
   ModuleSlotTracker MST(M);

llvm/lib/CodeGen/SelectionDAG/SelectionDAGPrinter.cpp

struct DOTGraphTraits<SelectionDAG*> : public DefaultDOTGraphTraits {
     static bool renderGraphFromBottomUp() {
       return true;
     }
 
     static std::string getNodeIdentifierLabel(const SDNode *Node,
                                               const SelectionDAG *Graph) {
       std::string R;
       raw_string_ostream OS(R);
-#ifndef NDEBUG
       OS << 't' << Node->PersistentId;
-#else
-      OS << static_cast<const void *>(Node);
-#endif
       return R;
     }
 
     /// If you want to override the dot attributes printed for a particular
     /// edge, override this method.
     template<typename EdgeIter>
     static std::string getEdgeAttributes(const void *Node, EdgeIter EI,
                                          const SelectionDAG *Graph) {