[PATCH] D114603: [DebugInfo][InstrRef] Terminate variable locations when overlapping fragments are assigned

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

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

One nit and an inline question, otherwise LGTM. The issue (if I've correctly identified an issue and not missed something) could be resolved in a separate patch IMO, so I'll hit Accept now.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1648
 /// known-to-overlap fragments are present".
 /// \param MI A previously unprocessed DEBUG_VALUE instruction to analyze for
 ///           fragment usage.
----------------
nit: possibly worth rewording to "A previously unprocessed debug instruction"?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1663
     OneFragment.insert(ThisFragment);
     SeenFragments.insert({MIVar.getVariable(), OneFragment});
 
----------------
Rather than having `DILocalVariable` as a key, shouldn't we be using `{DILocalVariable, InlinedAt}`? Otherwise, fragments of different inlined instances of the same variable may be considered overlapping, which is incorrect.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:731
+    // Otherwise: terminate any overlapped variable locations.
+    for (auto FragmentInfo : Overlaps->second) {
+      // The "empty" fragment is stored as DebugVariable::DefaultFragment, so
----------------
See other comment re: inlined-at and overlaps. Because of how the overlap map is constructed I think you're going to be treating fragments of 2 inlined instances of a variable as overlapping here. If I'm correct about that, I think the fix should be in how the overlap map is constructed  (see other comment).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114603