[PATCH] D44369: [SelectionDAG] Improve handling of dangling debug info

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Thanks! I have a few comments inline but otherwise this LGTM.



================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2420
+  /// Check if fragments overlap between this DIExpression and \p Other.
+  bool fragmentsOverlap(const DIExpression *Other) const {
+    if (!isFragment() || !Other->isFragment())
----------------
Could you unify this with DebugHandlerBase::fragmentsOverlap() and remove one of the two implementations?



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1082
+// dropDanglingDebugInfo - if we have dangling debug info that describes the
+// same variable (or part of variable) as DB, then we need to drop that debug
+// info, as it isn't valid any longer.
----------------
what is `DB`?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1083
+// same variable (or part of variable) as DB, then we need to drop that debug
+// info, as it isn't valid any longer.
+void SelectionDAGBuilder::dropDanglingDebugInfo(const DILocalVariable *Variable,
----------------
I know that this file isn't following the LLVM coding style, but for a new function I would still prefer the comment in the header file.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1110
   DanglingDebugInfo &DDI = DanglingDebugInfoMap[V];
   if (DDI.getDI()) {
     const DbgValueInst *DI = DDI.getDI();
----------------
while you are in here, could you replace this with an early exit?


Repository:
  rL LLVM

https://reviews.llvm.org/D44369