[DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope

[Orlando]

Summary

This patch reduces file size by dropping variable locations a debugger user
will not see.

Background

PR45889 [1] describes a bug where we get a variable location for an instruction
range that exists outside of the instruction range of the scope that it is
declared in.

We discovered the root cause of that particular problem lies in the handling of
parameters and argument values through SelectionDAG. Locations like these are
superfluous. Test llvm/test/DebugInfo/COFF/register-variables.ll shows the same
problem occuring as a result of a seemingly incorrect scope range (more on this
later). In cases like this the variable locations appear to be sane and it is
that the scope ranges are wrong or misleading.

A debugger user will not get to see these out of scope locations. Not only do
they increase the size of the binary in and of themselves, but having more than
one location will prevent us from emitting locations that cover their entire
scope as single locations. Single locations are desirable because they take up
less space than location lists with single entries.

Solution

This patch drops variable locations which exist entirely outside of the
variable's scope. The way it is implemented is simple: after building the debug
entity history map we loop through it. For each variable we look at each entry.
If the entry opens a location range which does not intersect any of the
variable's scope's ranges then we mark it for removal. After visiting the
entries for each variable we also mark any clobbering entries which will no
longer be referenced for removal, and then finally erase the marked entries.
This all requires the ability to query the order of instructions, so before
this runs we number them.

Results

Building CTMark with CMAKE_BUILD_TYPE=RelWithDebInfo without (base) and with
(patched) this patch yields a geomean binary size difference of -1.9%, with the
.debug_loc sections up to nearly 14% smaller in some cases. For a clang-3.4
build I see similar percentage savings to sqlite3 in the suite below.

Program                                        base      patched   diff 
 test-suite :: CTMark/SPASS/SPASS.test          3971320   3814544  -3.9%
 test-suite...ark/tramp3d-v4/tramp3d-v4.test    14357584  13812000 -3.8%
 test-suite...TMark/7zip/7zip-benchmark.test    9190760   8885512  -3.3%
 test-suite :: CTMark/Bullet/bullet.test        7403096   7182752  -3.0%
 test-suite...:: CTMark/sqlite3/sqlite3.test    4077552   4003072  -1.8%
 test-suite :: CTMark/kimwitu++/kc.test         5117728   5038336  -1.6%
 test-suite...:: CTMark/ClamAV/clamscan.test    2545312   2526184  -0.8%
 test-suite :: CTMark/lencod/lencod.test        2643960   2631568  -0.5%
 test-suite...Mark/mafft/pairlocalalign.test    1530904   1526760  -0.3%
 test-suite...-typeset/consumer-typeset.test    1527120   1524040  -0.2%
 Geomean difference                                                -1.9%

My machine is not set up for precise performance measurements, but I observed
no significant change in compile time (0.0x% change in either direction).

The function validThroughout in DwarfDebug.cpp returns true when a location can
be considered a single location for a variable. Earlier I mentioned that single
locations are desirable. Here are some numbers from a clang-3.4 build that show
the patch improving our abillity to detect them:

                                    base    patched
times validThroughout is called     3676550 3638269
times validThroughout returns true  1470517 1541733
percentage of calls returning true  40.0%   42.4%

For these benchmarks 'base' is clang at 772349de887, and 'patched' is that
commit with the patch applied on top.

Tests

Added llvm/test/DebugInfo/X86/trim-var-locs.mir

Modified llvm/test/DebugInfo/X86/live-debug-variables.ll
Modified llvm/test/DebugInfo/ARM/PR26163.ll
In both tests an out of scope location is now removed. The remaining location
covers the entire scope of the variable allowing us to emit it as a single
location.

Modified llvm/test/DebugInfo/COFF/register-variables.ll
Branch folding merges the tails of if.then and if.else into if.else. Each
blocks' debug-locations point to different scopes so when they're merged we
can't use either. Because of this the variable 'c' ends up with a location
range which doesn't cover any instructions in its scope; with the patch applied
the location range is dropped and its flag changes to IsOptimizedOut.

Related future work?

The simple instruction numbering added in this patch can be used to improve how
we detect single locations in validThroughout (DwarfDebug.cpp). With a small
change on top of this patch we can reduce binary size even further, and
potentially by a similar magnitude. In addition it allows us to replaces a
linear scan in validThroughout with a map lookup.

Summary

This patch reduces the binary size of RelWithDebInfo builds by an average of
1.9%, and by nearly 4% in one case, across 11 applications by dropping variable
locations which a debugger user will never see. Some of these locations exist
in the first place as a result of bugs in clang so there's an argument that
this patch should not land, and instead we should make a verifer pass (either
in clang and llc, or in llvm-dwarfdump), and focus on fixing the issues they
reveal. OTOH this patch has immediate and tangible wins, and doesn't preclude
work on fixing those fundamental issues.

What do you think?

Thank you for taking the time to read this,
Orlando

[1] https://bugs.llvm.org/show_bug.cgi?id=45889

[dblaikie]

Thanks for working this up/sending it out.

The textual description doesn't /sound/ like it handles partial overlap ("If the entry opens a location range which does not intersect any of the variable's scope's ranges then we mark it for removal.") how are they handled? Are locations trimmed down to match the scope range too?

I'm guessing they're actually covered and tested - overlap that extends beyond the end or start of the scope, etc.

Personally: I'm marginally in favor. Though I wouldn't mind seeing more data about cases where this turns up (which should be easier to find with this prototype) to see if they're readily fixable bugs.

Oh, also: did you try running llvm-dwarfdump statistics before/after? I believe it tracks number of bytes of variable location relative to the enclosing scope. Assuming it doesn't count bytes of variable location extending beyond the enclosing scope (if it does, that bug should be fixed - maybe flagging those "extra bytes" in a separate penalty bucket - and could have a separate penalty bucket for cases where a single location could've been used but a location list was used instead (either because of these extended scopes - or otherwise)) - the number of covered bytes should remain the same before/after this patch? Does it?

[djtodoro]

@Orlando Thanks for this!
As @dblaikie pointed out, I think this should not affect locations coverage, so it'd be nice if you check/share the results of either llvm-dwarfdump --statistics or llvm-locstats.

[Orlando]

@djtodoro, @dblaikie, thank you both for your comments.

In D82129#2102344, @dblaikie wrote:

Thanks for working this up/sending it out.

The textual description doesn't /sound/ like it handles partial overlap ("If the entry opens a location range which does not intersect any of the variable's scope's ranges then we mark it for removal.") how are they handled?

Partial overlaps are ignored for now. i.e. if the location range partially overlaps a scope range then we do not drop it.

Are locations trimmed down to match the scope range too?

Not in this patch - I have a TODO comment in there for exactly that though. The change required would be somewhat invasive so I thought it best to leave it for now.

I'm guessing they're actually covered and tested - overlap that extends beyond the end or start of the scope, etc.

They've been considered and handled in the code, but my test has room for improvement here.

Personally: I'm marginally in favor. Though I wouldn't mind seeing more data about cases where this turns up (which should be easier to find with this prototype) to see if they're readily fixable bugs.

I don't think these numbers are what you had in mind, but I had them to hand, and may be interesting?

clang-3.4 RelWithDebInfo -trim-var-locs=true build
variables analysed                3693356
variables with dropped locations  512874   (13.89% of variables)  
locations analysed                8798121
locations dropped                 665043   (7.56% of locations)

Oh, also: did you try running llvm-dwarfdump statistics before/after? I believe it tracks number of bytes of variable location relative to the enclosing scope. Assuming it doesn't count bytes of variable location extending beyond the enclosing scope (if it does, that bug should be fixed - maybe flagging those "extra bytes" in a separate penalty bucket - and could have a separate penalty bucket for cases where a single location could've been used but a location list was used instead (either because of these extended scopes - or otherwise)) - the number of covered bytes should remain the same before/after this patch? Does it?

Good idea. I tried this just now and was alarmed to see a difference in scope bytes covered! But then I had a look at the statistics code and IIUC it looks like no distinction is made between which scope the bytes cover:

in llvm/tools/llvm-dwarfdump/Statistics.cpp

 ...
 @ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-dwarfdump/Statistics.cpp#L280
    for (auto Entry : *Loc) {
      uint64_t BytesEntryCovered = Entry.Range->HighPC - Entry.Range->LowPC;
      BytesCovered += BytesEntryCovered;
      if (IsEntryValue(Entry.Expr))
        BytesEntryValuesCovered += BytesEntryCovered;
    }
...
@ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-dwarfdump/Statistics.cpp#L317
// Turns out we have a lot of ranges that extend past the lexical scope.
GlobalStats.ScopeBytesCovered += std::min(BytesInScope, BytesCovered);

[aprantl]

I'm a bit surprised how much code is necessary for this, but if there are clear size benefits of doing this, we should probably do it. Thanks for the detailed writeup!

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

100

Nit: It's nice to use Doxygen-style /// comments here, since IDEs can format it specially and integrate it in help browsers etc.

130

Is this an inclusive interval on both sides or should this be [StartMI, EndMI)?

202

this looks like it could be a range-based for loop?

270

range-based for?

[Orlando]

Hi @aprantl, thanks for taking a look.

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

130

This is meant to be inclusive on both sides, yeah. Looks like the second if inside the function body should actually be:

if (EndMI && !isBefore(RangesI->second, EndMI, Ordering))
   return RangesI;

I'll fix this mistake. Though luckily this won't have made a noticeable difference since AFAICT we're only incorrectly dropping 0 length location ranges because of it, and these wouldn't be emitted anyway.

202

I agree that this loop is a little bit ugly; I'm doing it this way to update StartIndex.

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

202

I see. I guess that is better than

for (auto EI : HistoryMapEntries) {
  LLVM_DEFER { StartIndex++; };
  ...

[Orlando]

Addressed comments from @aprantl

To continue to scope-bytes-covered conversation a little further: Even if we did report the coverage stat correctly (i.e. only parent scope bytes covered count) we'd still see slightly different results. There's at least one place I can think of where some special case code is triggered because of the fewer number of location ranges.

[probinson]

This patch depends on the ranges for all scopes to be (reasonably) correct, but I think there's one modified test where that's not the case; a variable location is dropped because (AFAICT looking at the equivalent DWARF output) its containing scope isn't pointing to the right instructions.
If we take the stance that these cases are bugs, then this analysis is better off done in a verifier, so we can find and fix those cases, rather than papering over compiler bugs.

I might be wrong about that test, and maybe we aren't going to take that stance, but I thought it was worth bringing up here.

llvm/test/DebugInfo/COFF/register-variables.ll

111

I believe this one is being marked as OptimizedOut because the containing scope is pointing to the wrong instructions. The variable's instruction range looks not unreasonable.

[jmorse]

FWIW: I'd enjoy this patch landing. AFAIUI there's no meaningful information communicated to the DWARF consumers by out-of-scope ranges (even if it indicates a compiler bug somewhere), and we may as well save them disk space and debugger-load-time.

If we're looking to make no-out-of-scope-ranges a verification property, it's probably best to use this code to produce warnings, then errors, then push further up the compilation pipeline. I'd much prefer connecting source/variable locations and designing this problem out, though.

llvm/test/DebugInfo/X86/trim-var-locs.mir

117

nit: scooooope

[Orlando]

In D82129#2107017, @probinson wrote:

This patch depends on the ranges for all scopes to be (reasonably) correct,

I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

but I think there's one modified test where that's not the case; a variable location is dropped because (AFAICT looking at the equivalent DWARF output) its containing scope isn't pointing to the right instructions.
If we take the stance that these cases are bugs, then this analysis is better off done in a verifier, so we can find and fix those cases, rather than papering over compiler bugs.

I might be wrong about that test, and maybe we aren't going to take that stance, but I thought it was worth bringing up here.

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

202

I wasn't aware of LLVM_DEFER (and I can't see it with grep?). I've left it as is for now but I'm happy to change it.

llvm/test/DebugInfo/X86/trim-var-locs.mir

117

I'll update this here if I need to make other changes, or when I land it otherwise, thanks!

[probinson]

In D82129#2110934, @Orlando wrote:

In D82129#2107017, @probinson wrote:

This patch depends on the ranges for all scopes to be (reasonably) correct,

I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

Either way, the question remains: when we find cases where we need to "clear away" something, is that a bug, or is this merely a cleanup pass? In the case of the test I commented on, it's a bug, and I'd rather not be hiding bugs.

[djtodoro]

In D82129#2111602, @probinson wrote:

In D82129#2110934, @Orlando wrote:

In D82129#2107017, @probinson wrote:

This patch depends on the ranges for all scopes to be (reasonably) correct,

I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

Either way, the question remains: when we find cases where we need to "clear away" something, is that a bug, or is this merely a cleanup pass? In the case of the test I commented on, it's a bug, and I'd rather not be hiding bugs.

+1.

[Orlando]

In D82129#2111602, @probinson wrote:

In D82129#2110934, @Orlando wrote:

In D82129#2107017, @probinson wrote:

This patch depends on the ranges for all scopes to be (reasonably) correct,

I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

Either way, the question remains: when we find cases where we need to "clear away" something, is that a bug, or is this merely a cleanup pass? In the case of the test I commented on, it's a bug, and I'd rather not be hiding bugs.

Looking closer at COFF/register-variables.ll, it doesn't look like a bug but instead another victim of how we model debug info. Before running -branch-folder (Control Flow Optimizer) all the instructions in the else block belong to the else block scope. The branch folder merges the common tails from 'then' and 'else' into 'else', merging the debug-locations while it does so. @jmorse summarised the situation well offline: Every time we call getMergedLocation, we are creating the conditions where this occurs, and eliminating it during compilation would be a high burden.

[probinson]

In D82129#2114150, @Orlando wrote:

Looking closer at COFF/register-variables.ll, it doesn't look like a bug but instead another victim of how we model debug info. Before running -branch-folder (Control Flow Optimizer) all the instructions in the else block belong to the else block scope. The branch folder merges the common tails from 'then' and 'else' into 'else', merging the debug-locations while it does so. @jmorse summarised the situation well offline: Every time we call getMergedLocation, we are creating the conditions where this occurs, and eliminating it during compilation would be a high burden.

And yet, the variable was allocated to a register, and the variable's location information pointed to the correct instruction range.
Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.

[jmorse]

Paul wrote:

And yet, the variable was allocated to a register, and the variable's location information pointed to the correct instruction range.
Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.

My understanding is that we haven't failed to represent the scope here, instead it's been destroyed by optimisation. Here's the MIR before -branch-folder, minus some long lines:

bb.1.if.then: 
 ; predecessors: %bb.0 
   liveins: $eax 
   DBG_VALUE $eax, $noreg, !"a", !DIExpression(), debug-location !27; t.cpp:11:9 line no:11 
   DBG_VALUE $eax, $noreg, !"a", !DIExpression(), debug-location !34; t.cpp:4:33 @[ t.cpp:12:13 ] line no:4 
   renamable $eax = nsw ADD32ri8 killed renamable $eax(tied-def 0), 1, implicit-def dead $eflags, debug-location !36; t.cpp:5:13 @[ t.cpp:12:13 ] 
   DBG_VALUE $eax, $noreg, !"b", !DIExpression(), debug-location !37; t.cpp:5:7 @ [ t.cpp:12:13 ] line no:5 
   ADD32mi8 $rip, 1, $noreg, @x, $noreg, 1, implicit-def dead $eflags, debug-location !38 :: (deref stuff), (more deref stuff); t.cpp:6:3 @[ t.cpp:12:13 ] 
   DBG_VALUE $eax, $noreg, !"b", !DIExpression(), debug-location !43; t.cpp:12:9 line no:12 
   $ecx = COPY killed renamable $eax, debug-location !44; t.cpp:13:5 
   SEH_Epilogue debug-location !44; t.cpp:13:5 
   $rsp = frame-destroy ADD64ri8 $rsp(tied-def 0), 32, implicit-def dead $eflags, debug-location !44; t.cpp:13:5 
   $rsi = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !44; t.cpp:13:5 
   TCRETURNdi64 @putint, 0, <regmask blah blah blah>, implicit $rsp, implicit $ssp, implicit $ecx, debug-location !44; t.cpp:13:5 
 
 bb.2.if.else: 
 ; predecessors: %bb.0 
   liveins: $eax 
   DBG_VALUE $eax, $noreg, !"c", !DIExpression(), debug-location !46; t.cpp:15:9 line no:15 
   $ecx = COPY killed renamable $eax, debug-location !47; t.cpp:16:5 
   SEH_Epilogue debug-location !47; t.cpp:16:5 
   $rsp = frame-destroy ADD64ri8 $rsp(tied-def 0), 32, implicit-def dead $eflags, debug-location !47; t.cpp:16:5 
   $rsi = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !47; t.cpp:16:5 
   TCRETURNdi64 @putint, 0, <regmask blah blah blah>, implicit $rsp, implicit $ssp, implicit $ecx, debug-location !47; t.cpp:16:5

And here it is after:

[More bits of bb.1]
  DBG_VALUE $eax, $noreg, !"b", !DIExpression(), debug-location !37; t.cpp:5:7 @ [ t.cpp:12:13 ] line no:5
  ADD32mi8 $rip, 1, $noreg, @x, $noreg, 1, implicit-def dead $eflags, debug-location !38 :: (deref stuff), (more deref stuff); t.cpp:6:3 @[ t.cpp:12:13 ]
  DBG_VALUE $eax, $noreg, !"b", !DIExpression(), debug-location !43; t.cpp:12:9 line no:12

bb.2.if.else:
; predecessors: %bb.0, %bb.1
  liveins: $eax
  DBG_VALUE $eax, $noreg, !"c", !DIExpression(), debug-location !46; t.cpp:15:9 line no:15
  $ecx = COPY killed renamable $eax, debug-location !DILocation(line: 0, scope: !19); t.cpp:0
  SEH_Epilogue debug-location !DILocation(line: 0, scope: !19); t.cpp:0
  $rsp = frame-destroy ADD64ri8 $rsp(tied-def 0), 32, implicit-def dead $eflags, debug-location !DILocation(line: 0, scope: !19); t.cpp:0
  $rsi = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !DILocation(line: 0, scope: !19); t.cpp:0
  TCRETURNdi64 @putint, 0, <regmask blah blah blah>, implicit $rsp, implicit $ssp, implicit $ecx, debug-location !DILocation(line: 0, scope: !19); t.cpp:0

The tail of both blocks have been de-duplicated into the same block (bb.2), from the COPY to ecx, to the tail call. For every pair of duplicate instructions, there was no agreement on the source location, so they've all been given a zero line-number with the scope set to the parent scope of each pair, so for the original source:

void f(int p) {
   if (p) {
     int a = getint();
     int b = inlineinc(a);
     putint(b);
   } else {
     int c = getint();
     putint(c);
   }
}

Those instructions are considered in the scope of the "if" rather than for either of the blocks. The patch here then decides that, because the variable location is outside of a scope where it makes sense, it should be deleted. When you say,

Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.

Is there something that we can be doing better to represent the scopes in this scenario? I think it comes back to being able to describe instructions that correspond to multiple source locations, which as far as I'm aware is an open question right now.

[dblaikie]

In D82129#2115249, @probinson wrote:

In D82129#2114150, @Orlando wrote:

Looking closer at COFF/register-variables.ll, it doesn't look like a bug but instead another victim of how we model debug info. Before running -branch-folder (Control Flow Optimizer) all the instructions in the else block belong to the else block scope. The branch folder merges the common tails from 'then' and 'else' into 'else', merging the debug-locations while it does so. @jmorse summarised the situation well offline: Every time we call getMergedLocation, we are creating the conditions where this occurs, and eliminating it during compilation would be a high burden.

Hmm - could you explain that in more detail? If we merge the locations both if and else scopes would cease to exist (since we can't represent that ambiguity), right? But the dbg.value doesn't use/care about its !dbg, so it continues existing/describing a variable location over some unrelated instructions?

Fair enough.

And yet, the variable was allocated to a register, and the variable's location information pointed to the correct instruction range.
Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.

This is I think a point of disagreement (between you and I) - I don't think it's useful to emit DWARF that describes variables outside their scope. I don't think any consumer should do anything useful with that data & it seems like wasted bytes to me. (name lookup wouldn't find the variable at any point where it has a location, etc)

If we've failed to track a variable's location within its scope, we shouldn't emit any location for it. I don't think that variable location information is correct if it's not in the scope of the variable - in this case, there is no scope of the variable (or its been reduced) - so no range of instructions over which to describe the location of the variable, since it's not in-scope.

If that's the case - that the merged instructions drop the scope and leave behind dbg.values that describe the variable even though it's not in scope - where the fix would be to remove the dbg.values (because now they describe the location of a variable outside that variable's scope) would be impractical/ie: it's cheaper to remove it later - then I'm OK with that.

Though I worry about that this would leave around a lot of dead dbg.value intrinsics, perhaps? That we'd be better off cleaning up earlier, not just for the sake of the resulting DWARF.

From my perspective it's a question of whether we should actively drop that erroneous debug info at the end now - knowing that (so far as I've seen in the discussion) all such instances of it /might/ be bugs (if someone can show one example where we don't consider it a bug in an LLVM optimization

[Orlando]

In D82129#2115399, @dblaikie wrote:

In D82129#2115249, @probinson wrote:

In D82129#2114150, @Orlando wrote:

Looking closer at COFF/register-variables.ll, it doesn't look like a bug but instead another victim of how we model debug info. Before running -branch-folder (Control Flow Optimizer) all the instructions in the else block belong to the else block scope. The branch folder merges the common tails from 'then' and 'else' into 'else', merging the debug-locations while it does so. @jmorse summarised the situation well offline: Every time we call getMergedLocation, we are creating the conditions where this occurs, and eliminating it during compilation would be a high burden.

Hmm - could you explain that in more detail? If we merge the locations both if and else scopes would cease to exist (since we can't represent that ambiguity), right? But the dbg.value doesn't use/care about its !dbg, so it continues existing/describing a variable location over some unrelated instructions?

That's right.

Fair enough.

And yet, the variable was allocated to a register, and the variable's location information pointed to the correct instruction range.
Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.

This is I think a point of disagreement (between you and I) - I don't think it's useful to emit DWARF that describes variables outside their scope. I don't think any consumer should do anything useful with that data & it seems like wasted bytes to me. (name lookup wouldn't find the variable at any point where it has a location, etc)

If we've failed to track a variable's location within its scope, we shouldn't emit any location for it. I don't think that variable location information is correct if it's not in the scope of the variable - in this case, there is no scope of the variable (or its been reduced) - so no range of instructions over which to describe the location of the variable, since it's not in-scope.

If that's the case - that the merged instructions drop the scope and leave behind dbg.values that describe the variable even though it's not in scope - where the fix would be to remove the dbg.values (because now they describe the location of a variable outside that variable's scope) would be impractical/ie: it's cheaper to remove it later - then I'm OK with that.

That summarises the situation as I see it, yeah.

Though I worry about that this would leave around a lot of dead dbg.value intrinsics, perhaps? That we'd be better off cleaning up earlier, not just for the sake of the resulting DWARF.

I don't think we could do it before isel. One reason being that in IR we only know where locations start and not exactly how far they extend which means we can't do very precise scope range overlap checks. I suppose it could be possible to do it earlier post-isel? Though trimming here at the end is very safe because we have the final program structure to work with; knowing nothing is going to move around afterwards is nice.

From my perspective it's a question of whether we should actively drop that erroneous debug info at the end now - knowing that (so far as I've seen in the discussion) all such instances of it /might/ be bugs (if someone can show one example where we don't consider it a bug in an LLVM optimization

I'm not sure I follow here, please could you rephrase this part?

[Orlando]

Does anyone have any concerns with this patch that they feel have not been addressed?

I've slightly reworded the patch description following the discussion on the nature of the changes to the register-variables.ll test.

[probinson]

I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest. So I withdraw the objection on that basis. DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions. This all makes me sad, but that's how DWARF is at the moment.

Is there still an open question about whether this wants to be a cleanup pass or a verifier check? I apologize for losing track.

[dblaikie]

In D82129#2134241, @probinson wrote:

I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest. So I withdraw the objection on that basis. DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions. This all makes me sad, but that's how DWARF is at the moment.

Is there still an open question about whether this wants to be a cleanup pass or a verifier check? I apologize for losing track.

My take on it is that it's probably not practical to do this as a cleanup - it'd mean any time we merge debug locations, etc, we'd have to go check for isolated variable locations that have become invalid.

(though, inversely: I worry that not cleaning up those variable locations might be a source of IR bloat and algorithmic scaling problems when the debug locations are scanned... )

[Orlando]

In D82129#2134241, @probinson wrote:

I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest. So I withdraw the objection on that basis. DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions. This all makes me sad, but that's how DWARF is at the moment.

Is there still an open question about whether this wants to be a cleanup pass or a verifier check? I apologize for losing track.

I think we have ruled out a MIR/IR verifier pass, but flagging it in llvm-dwarfdump somehow would still be nice and I wrote a ticket for fixing up the --statistics (PR46575). Instead, I think the question is now whether this should happen earlier in some way to reduce the number of redundant intrinsics, as David says in his reply below.

In D82129#2134609, @dblaikie wrote:

My take on it is that it's probably not practical to do this as a cleanup - it'd mean any time we merge debug locations, etc, we'd have to go check for isolated variable locations that have become invalid.

(though, inversely: I worry that not cleaning up those variable locations might be a source of IR bloat and algorithmic scaling problems when the debug locations are scanned... )

I chose to do the trimming here because I can say with confidence that it won't cause any coverage or correctness regressions. I agree that it would be nice to remove redundant intrinsics, though I'm not exactly sure what that implementation would entail without further investigation. Is anyone able to offer any insight on this? If not, I suppose it might be reasonable to attempt to do this earlier (in IR) to see if there are any problems, and compare the results. Though I won't be able to get on this for a little while myself.

[dblaikie]

In D82129#2135946, @Orlando wrote:

In D82129#2134241, @probinson wrote:

I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest. So I withdraw the objection on that basis. DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions. This all makes me sad, but that's how DWARF is at the moment.

Is there still an open question about whether this wants to be a cleanup pass or a verifier check? I apologize for losing track.

I think we have ruled out a MIR/IR verifier pass, but flagging it in llvm-dwarfdump somehow would still be nice and I wrote a ticket for fixing up the --statistics (PR46575). Instead, I think the question is now whether this should happen earlier in some way to reduce the number of redundant intrinsics, as David says in his reply below.

In D82129#2134609, @dblaikie wrote:

My take on it is that it's probably not practical to do this as a cleanup - it'd mean any time we merge debug locations, etc, we'd have to go check for isolated variable locations that have become invalid.

(though, inversely: I worry that not cleaning up those variable locations might be a source of IR bloat and algorithmic scaling problems when the debug locations are scanned... )

I chose to do the trimming here because I can say with confidence that it won't cause any coverage or correctness regressions. I agree that it would be nice to remove redundant intrinsics, though I'm not exactly sure what that implementation would entail without further investigation. Is anyone able to offer any insight on this? If not, I suppose it might be reasonable to attempt to do this earlier (in IR) to see if there are any problems, and compare the results. Though I won't be able to get on this for a little while myself.

I don't have any particular insight on that, no. If no one else is stepping up, this patch as-is (though I haven't reviewed the implementation in detail) seems like a reasonable improvement at least, and should be acceptable.

[Orlando]

Thanks everyone for the review and discussion so far. The general idea of the patch has been okayed and it just needs a code review now. Could anyone please take a look?

Thanks,
Orlando

[aprantl]

LGTM once Paul's comment is addressed.

[Orlando]

In D82129#2165231, @aprantl wrote:

LGTM once Paul's comment is addressed.

Thanks! If you're referring to Paul's inline comment in test llvm/test/DebugInfo/COFF/register-variables.ll we resolved that in the non-inline comments and I should've marked it as done, oops. I can't find another unaddressed comment so I'll land this soon.

llvm/include/llvm/CodeGen/DbgEntityHistoryCalculator.h

 //===- llvm/CodeGen/DbgEntityHistoryCalculator.h ----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_DBGVALUEHISTORYCALCULATOR_H
 #define LLVM_CODEGEN_DBGVALUEHISTORYCALCULATOR_H
 
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/LexicalScopes.h"
 #include <utility>
 
 namespace llvm {
 
 class DILocalVariable;
 class DILocation;
 class DINode;
 class MachineFunction;
public:
   ///   an instruction that clobbers the value.
   ///
   /// * Clobbering entry:
   ///
   ///   This entry's instruction clobbers one or more preceding
   ///   register-described debug values that have their end index
   ///   set to this entry's position in the entry vector.
   class Entry {
+    friend DbgValueHistoryMap;
+
   public:
     enum EntryKind { DbgValue, Clobber };
 
     Entry(const MachineInstr *Instr, EntryKind Kind)
         : Instr(Instr, Kind), EndIndex(NoEntry) {}
 
     const MachineInstr *getInstr() const { return Instr.getPointer(); }
     EntryIndex getEndIndex() const { return EndIndex; }
bool startDbgValue(InlinedEntity Var, const MachineInstr &MI,
                      EntryIndex &NewIndex);
   EntryIndex startClobber(InlinedEntity Var, const MachineInstr &MI);
 
   Entry &getEntry(InlinedEntity Var, EntryIndex Index) {
     auto &Entries = VarEntries[Var];
     return Entries[Index];
   }
 
+  /// Drop location ranges which exist entirely outside each variable's scope.
+  void trimLocationRanges(const MachineFunction &MF, LexicalScopes &LScopes);
   bool empty() const { return VarEntries.empty(); }
   void clear() { VarEntries.clear(); }
   EntriesMap::const_iterator begin() const { return VarEntries.begin(); }
   EntriesMap::const_iterator end() const { return VarEntries.end(); }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const;
 #endif

llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

 //===- llvm/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp -------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/DbgEntityHistoryCalculator.h"
 #include "llvm/ADT/BitVector.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/LexicalScopes.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"

 void DbgValueHistoryMap::Entry::endEntry(EntryIndex Index) {
   // For now, instruction ranges are not allowed to cross basic block
   // boundaries.
   assert(isDbgValue() && "Setting end index for non-debug value");
   assert(!isClosed() && "End index has already been set");
   EndIndex = Index;
 }
 
+using OrderMap = DenseMap<const MachineInstr *, unsigned>;
+/// Number instructions so that we can compare instruction positions within MF.
+/// Meta instructions are given the same nubmer as the preceding instruction.
+/// Because the block ordering will not change it is possible (and safe) to
+/// compare instruction positions between blocks.
+static void numberInstructions(const MachineFunction &MF, OrderMap &Ordering) {
+  // We give meta instructions the same number as the peceding instruction
+  // because this function is written for the task of comparing positions of
+  // variable location ranges against scope ranges. To reflect what we'll see
+  // in the binary, when we look at location ranges we must consider all
+  // DBG_VALUEs between two real instructions at the same position. And a
+  // scope range which ends on a meta instruction should be considered to end
+  // at the last seen real instruction. E.g.
+  //
+  //  1 instruction p      Both the variable location for x and for y start
+  //  1 DBG_VALUE for "x"  after instruction p so we give them all the same
+  //  1 DBG_VALUE for "y"  number. If a scope range ends at DBG_VALUE for "y",
+  //  2 instruction q      we should treat it as ending after instruction p
+  //                       because it will be the last real instruction in the
+  //                       range. DBG_VALUEs at or after this position for
+  //                       variables declared in the scope will have no effect.
+  unsigned position = 0;
+  for (const MachineBasicBlock &MBB : MF)
+    for (const MachineInstr &MI : MBB)
+      Ordering[&MI] = MI.isMetaInstruction() ? position : ++position;
+}
+
+/// Check if instruction A comes before B. Meta instructions have the same
+/// position as the preceding non-meta instruction. See numberInstructions for
+/// more info.
+static bool isBefore(const MachineInstr *A, const MachineInstr *B,
+                     const OrderMap &Ordering) {
+  return Ordering.lookup(A) < Ordering.lookup(B);
+}
+
+/// Check if the instruction range [StartMI, EndMI] intersects any instruction
+/// range in Ranges. EndMI can be nullptr to indicate that the range is
+/// unbounded. Assumes Ranges is ordered and disjoint. Returns true and points
+/// to the first intersecting scope range if one exists.
+static Optional<ArrayRef<InsnRange>::iterator>
+intersects(const MachineInstr *StartMI, const MachineInstr *EndMI,
+           const ArrayRef<InsnRange> &Ranges, const OrderMap &Ordering) {
+  for (auto RangesI = Ranges.begin(), RangesE = Ranges.end();
+       RangesI != RangesE; ++RangesI) {
+    if (EndMI && isBefore(EndMI, RangesI->first, Ordering))
+      return None;
+    if (EndMI && !isBefore(RangesI->second, EndMI, Ordering))
+      return RangesI;
+    if (isBefore(StartMI, RangesI->second, Ordering))
+      return RangesI;
+  }
+  return None;
+}
+
+void DbgValueHistoryMap::trimLocationRanges(const MachineFunction &MF,
+                                            LexicalScopes &LScopes) {
+  OrderMap Ordering;
+  numberInstructions(MF, Ordering);
+
+  // The indices of the entries we're going to remove for each variable.
+  SmallVector<EntryIndex, 4> ToRemove;
+  // Entry reference count for each variable. Clobbers left with no references
+  // will be removed.
+  SmallVector<int, 4> ReferenceCount;
+  // Entries reference other entries by index. Offsets is used to remap these
+  // references if any entries are removed.
+  SmallVector<size_t, 4> Offsets;
+
+  for (auto &Record : VarEntries) {
+    auto &HistoryMapEntries = Record.second;
+    if (HistoryMapEntries.empty())
+      continue;
+
+    InlinedEntity Entity = Record.first;
+    const DILocalVariable *LocalVar = cast<DILocalVariable>(Entity.first);
+
+    LexicalScope *Scope = nullptr;
+    if (const DILocation *InlinedAt = Entity.second) {
+      Scope = LScopes.findInlinedScope(LocalVar->getScope(), InlinedAt);
+    } else {
+      Scope = LScopes.findLexicalScope(LocalVar->getScope());
+      // Ignore variables for non-inlined function level scopes. The scope
+      // ranges (from scope->getRanges()) will not include any instructions
+      // before the first one with a debug-location, which could cause us to
+      // incorrectly drop a location. We could introduce special casing for
+      // these variables, but it doesn't seem worth it because no out-of-scope
+      // locations have been observed for variables declared in function level
+      // scopes.
+      if (Scope &&
+          (Scope->getScopeNode() == Scope->getScopeNode()->getSubprogram()) &&
+          (Scope->getScopeNode() == LocalVar->getScope()))
+        continue;
+    }
+
+    // If there is no scope for the variable then something has probably gone
+    // wrong.
+    if (!Scope)
+      continue;
+
+    ToRemove.clear();
+    // Zero the reference counts.
+    ReferenceCount.assign(HistoryMapEntries.size(), 0);
+    // Index of the DBG_VALUE which marks the start of the current location
+    // range.
+    EntryIndex StartIndex = 0;
+    ArrayRef<InsnRange> ScopeRanges(Scope->getRanges());
+    for (auto EI = HistoryMapEntries.begin(), EE = HistoryMapEntries.end();
+         EI != EE; ++EI, ++StartIndex) {
+      // Only DBG_VALUEs can open location ranges so skip anything else.
+      if (!EI->isDbgValue())
+        continue;
+
+      // Index of the entry which closes this range.
+      EntryIndex EndIndex = EI->getEndIndex();
+      // If this range is closed bump the reference count of the closing entry.
+      if (EndIndex != NoEntry)
+        ReferenceCount[EndIndex] += 1;
+      // Skip this location range if the opening entry is still referenced. It
+      // may close a location range which intersects a scope range.
+      // TODO: We could be 'smarter' and trim these kinds of ranges such that
+      // they do not leak out of the scope ranges if they partially overlap.
+      if (ReferenceCount[StartIndex] > 0)
+        continue;
+
+      const MachineInstr *StartMI = EI->getInstr();
+      const MachineInstr *EndMI = EndIndex != NoEntry
+                                      ? HistoryMapEntries[EndIndex].getInstr()
+                                      : nullptr;
+      // Check if the location range [StartMI, EndMI] intersects with any scope
+      // range for the variable.
+      if (auto R = intersects(StartMI, EndMI, ScopeRanges, Ordering)) {
+        // Adjust ScopeRanges to exclude ranges which subsequent location ranges
+        // cannot possibly intersect.
+        ScopeRanges = ArrayRef<InsnRange>(R.getValue(), ScopeRanges.end());
+      } else {
+        // If the location range does not intersect any scope range then the
+        // DBG_VALUE which opened this location range is usless, mark it for
+        // removal.
+        ToRemove.push_back(StartIndex);
+        // Because we'll be removing this entry we need to update the reference
+        // count of the closing entry, if one exists.
+        if (EndIndex != NoEntry)
+          ReferenceCount[EndIndex] -= 1;
+      }
+    }
+
+    // If there is nothing to remove then jump to next variable.
+    if (ToRemove.empty())
+      continue;
+
+    // Mark clobbers that will no longer close any location ranges for removal.
+    for (size_t i = 0; i < HistoryMapEntries.size(); ++i)
+      if (ReferenceCount[i] <= 0 && HistoryMapEntries[i].isClobber())
+        ToRemove.push_back(i);
+
+    std::sort(ToRemove.begin(), ToRemove.end());
+
+    // Build an offset map so we can update the EndIndex of the remaining
+    // entries.
+    // Zero the offsets.
+    Offsets.assign(HistoryMapEntries.size(), 0);
+    size_t CurOffset = 0;
+    auto ToRemoveItr = ToRemove.begin();
+    for (size_t EntryIdx = *ToRemoveItr; EntryIdx < HistoryMapEntries.size();
+         ++EntryIdx) {
+      // Check if this is an entry which will be removed.
+      if (ToRemoveItr != ToRemove.end() && *ToRemoveItr == EntryIdx) {
+        ++ToRemoveItr;
+        ++CurOffset;
+      }
+      Offsets[EntryIdx] = CurOffset;
+    }
+
+    // Update the EndIndex of the entries to account for those which will be
+    // removed.
+    for (auto &Entry : HistoryMapEntries)
+      if (Entry.isClosed())
+        Entry.EndIndex -= Offsets[Entry.EndIndex];
+
+    // Now actually remove the entries. Iterate backwards so that our remaining
+    // ToRemove indices are valid after each erase.
+    for (auto Itr = ToRemove.rbegin(), End = ToRemove.rend(); Itr != End; ++Itr)
+      HistoryMapEntries.erase(HistoryMapEntries.begin() + *Itr);
+  }
+}
+
 void DbgLabelInstrMap::addInstr(InlinedEntity Label, const MachineInstr &MI) {
   assert(MI.isDebugLabel() && "not a DBG_LABEL");
   LabelInstr[Label] = &MI;
 }
 
 namespace {
 
 // Maps physreg numbers to the variables they describe.

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

 #include "llvm/ADT/Twine.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/MC/MCStreamer.h"
+#include "llvm/Support/CommandLine.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "dwarfdebug"
 
+/// If true, we drop variable location ranges which exist entirely outside the
+/// variable's lexical scope instruction ranges.
+static cl::opt<bool> TrimVarLocs("trim-var-locs", cl::Hidden, cl::init(true));
+
 Optional<DbgVariableLocation>
 DbgVariableLocation::extractFromMachineInstruction(
     const MachineInstr &Instruction) {
   DbgVariableLocation Location;
   if (!Instruction.isDebugValue())
     return None;
   if (!Instruction.getDebugOperand(0).isReg())
     return None;
void DebugHandlerBase::beginFunction(const MachineFunction *MF) {
   // Make sure that each lexical scope will have a begin/end label.
   identifyScopeMarkers();
 
   // Calculate history for local variables.
   assert(DbgValues.empty() && "DbgValues map wasn't cleaned!");
   assert(DbgLabels.empty() && "DbgLabels map wasn't cleaned!");
   calculateDbgEntityHistory(MF, Asm->MF->getSubtarget().getRegisterInfo(),
                             DbgValues, DbgLabels);
+  if (TrimVarLocs)
+    DbgValues.trimLocationRanges(*MF, LScopes);
   LLVM_DEBUG(DbgValues.dump());
 
   // Request labels for the full history.
   for (const auto &I : DbgValues) {
     const auto &Entries = I.second;
     if (Entries.empty())
       continue;
 

llvm/test/DebugInfo/ARM/PR26163.ll

 ; RUN: llc -filetype=obj -o - < %s | llvm-dwarfdump -debug-info - | FileCheck %s
 ;
 ; Checks that we're omitting the first range, as it is empty, and that we're
 ; emitting one that spans the rest of the function. In this case, the first
 ; range, which we omit, describes 8 bytes of the variable using DW_OP_litX,
 ; whereas the second one only describes 4 bytes, so clobbering the whole 8 byte
 ; fragment with the 4 bytes fragment isn't necessarily best thing to do here,
 ; but it is what is currently being emitted. Any change here needs to be
 ; intentional, so the test is very specific.
 ;
-; CHECK: DW_TAG_inlined_subroutine
-; CHECK: DW_TAG_variable
-; CHECK:   DW_AT_location ({{.*}}
-; CHECK-NEXT: [0x00000004, 0x00000014): DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4)
+; The variable is given a single location instead of a location list entry
+; because the function validThroughout has a special code path for single
+; locations with a constant value that start in the prologue.
+;
+; CHECK:      DW_TAG_inlined_subroutine
+; CHECK:        DW_TAG_variable
+; CHECK-NEXT:     DW_AT_location (DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x4)
+; CHECK-NEXT      DW_AT_name ("i4")
 
 ; Created form the following test case (PR26163) with
 ; clang -cc1 -triple armv4t--freebsd11.0-gnueabi -emit-obj -debug-info-kind=standalone -O2 -x c test.c
 ;
 ; typedef unsigned int  size_t;
 ; struct timeval {
 ;   long long tv_sec;
 ;   int tv_usec;

llvm/test/DebugInfo/COFF/register-variables.ll

 ; ASM: [[func_finished:\.Ltmp.*]]:
 
 ; ASM:         .short  4414                    # Record kind: S_LOCAL
 ; ASM:         .asciz  "p"
 ; ASM:         .cv_def_range    .Lfunc_begin0 [[p_ecx_esi]], reg, 18
 ; ASM:         .cv_def_range    [[p_ecx_esi]] [[func_end]], reg, 23
 ; ASM:         .short  4414                    # Record kind: S_LOCAL
 ; ASM:         .asciz  "c"
-; ASM:         .cv_def_range    [[after_if]] [[func_finished]], reg, 17
 ; ASM:         .short  4414                    # Record kind: S_LOCAL
 ; ASM:         .asciz  "a"
 ; ASM:         .cv_def_range    [[after_je]] [[after_inc_eax]], reg, 17
 ; ASM:         .short  4414                    # Record kind: S_LOCAL
 ; ASM:         .asciz  "b"
-; ASM:         .cv_def_range    [[after_if]] [[after_if]], reg, 17
 
 ; Note: "b" is a victim of tail de-duplication / branch folding.
 
 ; ASM:         .short  4429                    # Record kind: S_INLINESITE
 ; ASM:         .short  4414                    # Record kind: S_LOCAL
 ; ASM:         .asciz  "a"
 ; ASM:         .cv_def_range    [[after_je]] [[after_inc_eax]], reg, 17
 ; ASM:         .short  4414                    # Record kind: S_LOCAL

 ; OBJ:     LocalVariableAddrRange {
 ; OBJ:       OffsetStart: .text+0x7
 ; OBJ:       ISectStart: 0x0
 ; OBJ:       Range: 0x1A
 ; OBJ:     }
 ; OBJ:   }
 ; OBJ:   LocalSym {
 ; OBJ:     Type: int (0x74)
-; OBJ:     Flags [ (0x0)
+; OBJ:     Flags [ (0x100)
+; OBJ:       IsOptimizedOut (0x100)
 ; OBJ:     ]
 ; OBJ:     VarName: c
 ; OBJ:   }
-; OBJ:   DefRangeRegisterSym {
-; OBJ:     Register: EAX (0x11)
-; OBJ:     LocalVariableAddrRange {
-; OBJ:       OffsetStart: .text+0x1A
-; OBJ:       ISectStart: 0x0
-; OBJ:       Range: 0xC
-; OBJ:     }
-; OBJ:   }
 ; OBJ:   LocalSym {
 ; OBJ:     Type: int (0x74)
 ; OBJ:     Flags [ (0x0)
 ; OBJ:     ]
 ; OBJ:     VarName: a
 ; OBJ:   }
 ; OBJ:   DefRangeRegisterSym {
 ; OBJ:     Register: EAX (0x11)

llvm/test/DebugInfo/X86/live-debug-variables.ll

-; RUN: llc -mtriple=x86_64-linux-gnu -filetype=obj -o - %s | llvm-dwarfdump -debug-loc - | FileCheck %s
+; RUN: llc -mtriple=x86_64-linux-gnu -filetype=obj -o - %s | llvm-dwarfdump -name i4 - \
+; RUN:     | FileCheck %s
 
 ; The test inlines the function F four times, with each inlined variable for
 ; "i4" sharing the same virtual register. This means the live interval of the
 ; register spans all of the inlined callsites, extending beyond the lexical
 ; scope of each. Later during register allocation the live interval is split
 ; into multiple intervals. Check that this does not generate multiple entries
 ; within the debug location (see PR33730).
 ;
 ; Generated from:
 ;
 ; extern int foobar(int, int, int, int, int);
 ;
 ; int F(int i1, int i2, int i3, int i4, int i5) {
 ;   return foobar(i1, i2, i3, i4, i5);
 ; }
 ;
 ; int foo(int a, int b, int c, int d, int e) {
 ;   return F(a,b,c,d,e) +
 ;          F(a,b,c,d,e) +
 ;          F(a,b,c,d,e) +
 ;          F(a,b,c,d,e);
 ; }
 
-; CHECK:      .debug_loc contents:
-; CHECK-NEXT: 0x00000000:
-;   We currently emit an entry for the function prologue, too, which could be optimized away.
-; CHECK:              (0x0000000000000018, 0x0000000000000072): DW_OP_reg3 RBX
-;   We should only have one entry inside the function.
-; CHECK-NOT: :
+; Ignore the abstract entry.
+; CHECK: DW_TAG_formal_parameter
+; Check concrete entry has a single location.
+; CHECK:      DW_TAG_formal_parameter
+; CHECK-NEXT:   DW_AT_location (DW_OP_reg3 RBX)
+; CHECK-NEXT:   DW_AT_abstract_origin
+; CHECK-NOT:  DW_TAG_formal_parameter
 
 declare i32 @foobar(i32, i32, i32, i32, i32)
 
 define i32 @foo(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e) !dbg !25 {
 entry:
   tail call void @llvm.dbg.value(metadata i32 %d, i64 0, metadata !15, metadata !17) #3, !dbg !41
   %call.i = tail call i32 @foobar(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e) #3, !dbg !43
   %call.i21 = tail call i32 @foobar(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e) #3, !dbg !50

llvm/test/DebugInfo/X86/trim-var-locs.mir

+# RUN: llc %s --start-after=livedebugvalues -filetype=obj -o - \
+# RUN:     | llvm-dwarfdump - -name local* -regex \
+# RUN:     | FileCheck %s
+#
+# Test that the -trim-var-locs option (enabled by default) works correctly.
+# Test directives and comments inline.
+
+--- |
+  target triple = "x86_64-unknown-linux-gnu"
+  define dso_local i32 @fun() local_unnamed_addr !dbg !7 {
+  entry:
+    ret i32 0
+  }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "example.c", directory: "/")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 11.0.0"}
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !11 = !{!12, !13, !25}
+  !22 = !DISubroutineType(types: !23)
+  !23 = !{!10, !10}
+  ; --- Important metadata ---
+  !7 = distinct !DISubprogram(name: "fun", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+  !24 = distinct !DILexicalBlock(scope: !7, file: !1, line: 9, column: 3)
+  !14 = distinct !DILexicalBlock(scope: !7, file: !1, line: 4, column: 3)
+  !12 = !DILocalVariable(name: "locala", scope: !7, file: !1, line: 1, type: !10)
+  !13 = !DILocalVariable(name: "localb", scope: !14, file: !1, line: 2, type: !10)
+  !25 = !DILocalVariable(name: "localc", scope: !24, file: !1, line: 3, type: !10)
+  !15 = !DILocation(line: 1, column: 0, scope: !7)
+  !18 = !DILocation(line: 2, column: 1, scope: !14)
+  !26 = !DILocation(line: 3, column: 1, scope: !24)
+...
+---
+name:            fun
+body:             |
+  bb.0.entry:
+    ; This is the scope and variable structure:
+    ; int fun() {       // scope fun !7
+    ;   int locala;     // scope fun !7,        var locala !12, debug-location !15
+    ;   { int localb; } // scope fun:block !14, var localb !13, debug-location !18
+    ;   { int localc; } // scope fun:block !24, var localc !25, debug-location !26
+    ; }
+    ;
+    ; (1) Check that a variable location range found in implied scope fun !7 is
+    ;     not trimmed.
+    ;
+    ; CHECK:      DW_TAG_variable
+    ; CHECK-NEXT:   DW_AT_location
+    ; CHECK-NEXT:     DW_OP_reg0 RAX
+    ; CHECK-NEXT:   DW_AT_name ("locala")
+    ;
+    ; scope fun !7 is implied as we're in function fun and haven't seen a debug-location
+    $eax = MOV32ri 0
+    ; locala range 1 start in implicit scope fun !7
+    DBG_VALUE $eax, $noreg, !12, !DIExpression(), debug-location !15
+    $edi = MOV32ri 1
+    ; locala range 1 clobber in implicit scope fun !7
+    $eax = MOV32ri 2
+    ; scope fun !7 explicit start
+    $edi = MOV32ri 3, debug-location !15
+
+    ; (2) Check that a variable location range found outside lexical block is
+    ;     trimmed. See check directives for (3).
+    ;
+    ; localb range 1 start in scope fun !7 (outside block !14).
+    DBG_VALUE $eax, $noreg, !13, !DIExpression(), debug-location !18
+    ; localb range 1 clobber in scope fun !7
+    $edi = MOV32ri 4, debug-location !15
+
+    ; (3) Check that a variable location range which overlaps the entire lexical
+    ;     block is not trimmed.
+    ;
+    ; CHECK:      DW_TAG_variable
+    ; CHECK-NEXT:   DW_AT_location
+    ; CHECK-NEXT:     DW_OP_reg5 RDI
+    ; CHECK-NEXT:   DW_AT_name ("localb")
+    ;
+    ; localb range 2 clobber in scope fun !7 (outside block !14)
+    DBG_VALUE $edi, $noreg, !13, !DIExpression(), debug-location !18
+    ; scope block !14 start (and only instruction)
+    $edi = MOV32ri 5, debug-location !18
+
+    ; (4) Check that a variable location range in scope fun !7 (outside block
+    ;     !14) is trimmed. See check directives for (3).
+    ;
+    ; localb range 3 starts after scope !14 (prev instr is last in scope)
+    DBG_VALUE $rax, $noreg, !13, !DIExpression(), debug-location !18
+    ; scope block !14 end
+    $edi = MOV32ri 6, debug-location !15
+
+    ; (5) Check that a variable location range found between disjoint scope
+    ;     ranges is trimmed.
+    ;
+    ; CHECK:      DW_TAG_variable
+    ; CHECK-NOT:    DW_AT_location
+    ; CHECK-NEXT:   DW_AT_name ("localc")
+    ;
+    ; scope fun !7
+    $edi = MOV32ri 6, debug-location !15
+    ; scope block !24 start and end range 1
+    $edi = MOV32ri 7, debug-location !26
+    ; localc range 1 start in scope !7
+    DBG_VALUE $edi, $noreg, !25, !DIExpression(), debug-location !18
+    ; localc range 1 clobber in scope !7
+    $edi = MOV32ri 8, debug-location !15
+    ; scope block !24 start and end range 2
+    $edi = MOV32ri 9, debug-location !26
+
+    ; scope fun !7
+    RETQ debug-location !15
+...