Thanks @rnk

As @bjope points out on D91425, it should be mentioned that while solution will improve the debugging experience in a number of cases it isn't bulletproof. For example a pass could insert a non-debug instruction between the dbg.value+deref and the to-be-inlined call.

Address feedback from @bjope:
+ Change comment for dbg.value+derefs in ShouldRemove lambda.
+ Update the test to check that the backward scan removes adjacent duplicate dbg.value+derefs still.

Rebase

Rebase and fix a typo in a comment.

Updated based on @aprantl's comments. I removed LowerDbgDeclare's doxygen comment from Local.cpp (it still exists in the header Local.h). I didn't add a doxygen comment to the new function ConvertIndirectDbgIntrinsicToDbgValues because it is added in the header in the next patch D91424.

Thanks @aprantl for looking at this. I meant to state in the description that this NFC change is set up for D91424 to help highlight the functional change there.

Thank you @bjope for taking a look at this.

Jeremy said:

In theory this kind of behaviour is exactly what dbg.addr is for (variable lives in memory, maybe changes due to control flow). However moving everything to use dbg.addr stalled a long time ago, I believe @Orlando found that it's often unexpectedly dropped by optimisations, any opinion on whether it's usable @Orlando? We're hoping to use it for things someday soon, but we're definitely not there yet.

Thanks. I tried running these on Windows but ran into issues with the dexter args so I landed these with UNSUPPORTED: system-windows.

Addressed Reid's comments:
+ Converted all tests to use attributes to control inlining behaviour (as opposed to a mix of this and clang flags).
+ Improved comments in some tests.

Apologies, in shortening the commit message I carelessly removed the review number. This was committed in fea067bdfde4.

Addressed @vsk's comments:
+ Resize SmallVector<...,8> -> SmallVector<...,1>
+ Rename Declares -> DbgUsers.

Hi @vsk, thanks for taking a look, I'll address your inline comments shortly.

FWIW, ignoring the test style discussion, the change LGTM.
AFAICT, prior to D80264 these dbg.value+derefs would've been silently dropped instead of made undef, so we're really just returning to previous behaviour with this patch. I've got a patch brewing which applies this fix to mem2reg which I'll upload shortly.

Thanks for taking a look @rnk. Your comment on struct-dse.c reminds me that I left out some important info in the description. All of these except ctrl-flow.c "fail" (score < 1.0) at the moment. Should they be marked XFAIL?

I guess all of these measurements were done without Split DWARF (shouldn't make things better/worse overall (across .o and .dwo), really, compared to DWARFv5 non-split - but means when only looking at .o files the difference of avoiding more debug_addr entries is more significant because there's fewer remaining .o debug bytes to begin with) and without compression (-gz) enabled?

Just to clarify: when I say "elf" here I'm talking about the linked executable file, and "object files" are the pre-link .o files.

The following builds are with clang @ 485e6db8729 (3rd September) targeting x86.

+---------------------------------------------------------------+
| File size (bytes) of clang-3.4 built with -O2 -gdwarf-4       |
*===============================================================*
|                    | base addr    | no base addr | % change   |
+--------------------+--------------+--------------+------------+
| Accumulated object | 1874653208   | 1924003152   | +2.63      |
| file sizes         |              |              |            |
+--------------------+--------------+--------------+------------+
| Elf size           | 527591064    | 513946184    | -2.57      |
+--------------------+--------------+--------------+------------+

Out of curiosity I also did a clang-3.4 build too using master @ 485e6db8729 (3rd September) with "-O2 -gdwarf-4". It is smaller when disabling base address specifiers (and emitting DWARFv 4) too:
With base addresses (default): Total File Size: 527591064
Without base addresses: Total File Size: 513946184 (-2.59 %)

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.