I have here a copy of the table I shared earlier, with a new row "ShouldUseBaseAddress=false". The stats for this row are taken at 57d8acac64b (D86153) with the changes mentioned in my previous comment (disabling base address specifiers).

+------------------------------------------------------------------------------------------- +
| Mean binary size for benchmarks normalized as a percentage of llvm-3 builds                |
+---------------------------------+------------+------------------+-----------------+--------+
| llvm version                    | .debug_loc | other debug info | everything else | Total  |
+---------------------------------+------------+------------------+-----------------+--------+
| llvm-3                          | 13.7       | 33.2             | 53.1            | 100    |
| llvm-4                          | 12.7       | 33.8             | 53.8            | 100.3  |
| llvm-5                          | 13.4       | 35.6             | 54.6            | 103.7  |
| llvm-7                          | 18.4       | 35.6             | 54.0            | 108.0  |
| llvm-8                          | 17.5       | 37.1             | 54.5            | 109.1  |
| llvm-9                          | 19.7       | 37.2             | 54.6            | 111.5  |
| llvm-10 before dblaikie commit  | 19.8       | 37.4             | 54.9            | 112.1  |
| llvm-10 with dblaikie commit    | 25.6       | 37.4             | 54.9            | 117.9  |
| llvm-10                         | 25.8       | 37.5             | 54.8            | 118.1  |
| llvm-master before my commits   | 26.2       | 37.4             | 54.8            | 118.4  |
| llvm-master with my commits     | 18.4       | 35.5             | 55.3            | 109.3  |
| ShouldUseBaseAddress=false      | 14.9       | 35.5             | 55.3            | 105.7  |
+---------------------------------+------------+------------------+-----------------+--------+

Ah, OK - so it sounds like we're back down below the size before I added debug_loc base address specifiers? That's good to hear!

The real thing exists over at D82129 (landed).

Thanks for the data!

@Orlando mentioned he was collecting some size data that would be relevant here, he'll post it when he's done. Basically .debug_loc sizes at various points.

I've reverted this for now (485e6db8729) because one of our internal build bot is failing in the same way. I don't think many (any?) of the bots on http://lab.llvm.org:8011/console build the debuginfo-tests project!

Before landing I removed the XFAIL:* from llvm/test/DebugInfo/AArch64/inlined-argument.ll because it passes with this patch.

@djtodoro do you have any more thoughts on this patch? Copying part of my inline comment here for any other reviewers:

A small update to address comments from @djtodoro.

Oops, I didn't include the Phabricator revision line in the commit message.

Hi @djtodoro thanks for taking a look!

+ Rebase
+ Update test for latest yaml2obj with change suggested by @Higuoxing (thank you!).
+ Make the test more interesting, @dblaikie wdyt?

Just a note: This patch moves the InstructionOrdering class added in D86150 out from DbgEntityHistoryCalculator.cpp to DbgEntityHistoryCalculator.h so that we're able to use it in DebugHandlerBase.

Thanks for the review! I'll land the patch alongside the others in the stack when they're good to go.

Hi, sorry for leaving this for so long! I think it will take me a couple of passes to fully understand it, but for now I left some inline nits/questions. Please feel free to leave the nit changes until later revisions.

Thanks @jhenderson. I think this should have also addressed @dblaikie's concerns regarding the test, so I will aim to land this in a couple of days if there are no objections.

Thank you @aprantl.

Thank you so much @Higuoxing, that's very kind!

Thanks @aprantl and @jhenderson.

Hi @dstenb, thanks for taking a look. I've updated the diff to include a test but it feels a little unwieldy. Is there a better way to test this?

Hi @vsk, just a inline question from me. And in addition, I noticed that SourceLevelDebugging specifically states "there can only be one call to llvm.dbg.declare for a given concrete local variable". I wonder if we should update this to say something like "there can only be one call to llvm.dbg.declare for a given fragment of a local variable"?

In D82129#2165231, @aprantl wrote:

LGTM once Paul's comment is addressed.

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?

Hey, just noticed a couple of comments to remove from the tests.

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.

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

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?

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.

In D82129#2107017, @probinson wrote:

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

Addressed comments from @aprantl

Hi @aprantl, thanks for taking a look.

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

Sorry for the delay, LGTM. Please wait 24h to see if anyone else has any further comments.

@Orlando I think you are probably right. It should indeed prints $0=1. I appreciate your patience and my apology for the report. Shall I close the original PR?

In D80691#2067707, @helloqirun wrote:

Thanks @Orlando, I think I agree with your reasoning. My patch on introducing an undef is based on the same reasoning (but I could be totally wrong).

Let's see what we should do instead.

Before earlycse, we have:

  %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
  call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31
  %1 = load i32, i32* @a, align 4, !dbg !32, !tbaa !27
  call void @llvm.dbg.value(metadata i32 %1, metadata !22, metadata !DIExpression()), !dbg !31

!26 = !DILocation(line: 5, column: 11, scope: !17)
!31 = !DILocation(line: 0, scope: !17)
!32 = !DILocation(line: 6, column: 15, scope: !17)

after earlycse, we have:

  %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
; I omitted the variable !21 here since it's irrelevant.
  call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31
  call void @llvm.dbg.value(metadata i32 %0, metadata !22, metadata !DIExpression()), !dbg !31

!21 = !DILocalVariable(name: "d", scope: !17, file: !3, line: 5, type: !12)
!22 = !DILocalVariable(name: "l_52", scope: !17, file: !3, line: 5, type: !12)
!26 = !DILocation(line: 5, column: 11, scope: !17)
!31 = !DILocation(line: 0, scope: !17)
!32 = !DILocation(line: 6, column: 20, scope: !17)

Could you please help me verify the following?
Based on my understanding, it seems that when eliminating the second load i32, i32* @a in EarlyCSE, we should use the location !32 instead of !26 for the resulting load i32, i32* @a.
Thanks.

This makes sense to me.

LGTM. There are two other text only LGTMs on this patch and AFAICT the only blocker is the (inline) discussion on whether salvageDebugInfo needs renaming, but there doesn't seem to be a consensus. Does anyone have a strong opinion on this subject?

This is quite a long reply so I've got a very small "summary" at the end.

I think that this patch reduces variable coverage by undefing dbg.values which
it cannot salvage that would otherwise be RAUWd.

I had another look at this and followed it up on the ticket (PR46114).

I had another look at this and followed it up on the ticket (PR46114).

Thanks for working on this!

I left some nits inline but otherwise I think this is good.

Thank you both, and @jmorse for doing a lot of the heavy lifting on this before I got to it.

+ Addressed your inline comments.

I've updated the diff to include 'single-location-inlined-param.mir' and 'single-location-interrupted-scope.mir' which should cover both of those cases IIUC.

Hey @Tyker it looks like you've picked up my last commit message here so you'll need to close your differential revision manually.

Thank you both for taking a look.

Thanks, landing shortly.