[PATCH] D120714: [CodeGen] Place SDNode debug ID declaration under ifndef

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a subscriber: bogner.
dexonsmith added a comment.
This revision now requires changes to proceed.

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).



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:279
 
+#ifndef NDEBUG
+  // Used for debug printing.
----------------
These should all be `#if LLVM_ENABLE_ABI_BREAKING_CHECKS`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120714/new/

https://reviews.llvm.org/D120714