Updated for the comments from @vsk . Deleted definition and usage of function LdStHasDebugValue.

In D74030#1892727, @vsk wrote:

In D74030#1891319, @alok wrote:

In D74030#1889750, @vsk wrote:

In D74030#1888655, @alok wrote:

Now,

1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)

Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.

Yes for me also it seems a bit difficult decision. There were few reasons which made me think to keep 1)

a) It looks clearer to avoid generating than first let it generate and later delete when there is no performance penalty. In case of avoiding generation of dbg.value for same load/store again and again there is a pattern (immediate previous/next instruction in question).

I'm not sure I understand how the IR could look clearer. It shouldn't make any difference if the redundant instructions are eliminated after all insertions occur. Unless there's some kind of significant compile time reduction to be had by keeping LdStHasDebugValue, I think we should get rid of it, as it'd just be adding unnecessary complexity.

b) There is existing functionality for avoiding generation in function named "LdStHasDebugValue", though the name suggests covering both load and store both but it had handling only for store, adding the case for load justifies its name and completes the functionality. Either we should remove this function completely or complete it.

Agreed! Strong +1 to this, it's just that I suspect deleting LdStHasDebugValue is the simpler way to go.

In D74030#1889750, @vsk wrote:

In D74030#1888655, @alok wrote:

Now,

1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)

Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.

This is updated to re-base and addressing common concern about performance and suggestion form @vsk to use RemoveRedundantDbgInstrs.

In D74030#1888527, @vsk wrote:

@alok I had a quick stab at this to see what it would take, and the result turned out to be relatively clean: https://github.com/vedantk/llvm-project/commit/85d7fe06810fa7f6166508be73eef80f676b0fc6.diff. This handles the situation in your 'duplicate_dbgvalue.ll' test case. Lmk what you think. Feel free to commandeer the patch if you'd like, or if you don't have the bandwidth for that I can kick off a fresh review. Thanks!

Updated to incorporate review comments from Adrian (@aprantl) .

In D74030#1869863, @dstenb wrote:

Thanks! No further comments from me.

I'll leave a chance for others that are more familiar with this code to have their say though.

This version is updated to incorporate comments from David (@dstenb ).

Hi David (@dstenb), I have incorporated all your comments. I have also added you as reviewer. Kindly give your feedback.

Updated to incorporate comments from @dstenb

Re-based and cleanup.

Re-based and some cleanup.

Updated to re-base and include a test case as suggested by @aprantl.

In D70643#1769810, @aprantl wrote:

Can you please add an assembler round-trip test to test/IR that runs llvm-as | llvm-dis | llvm-as - | llvm-dis ? There are a bunch of examples in that directory.

This is re-based and updated to apply LLVM_arg0 to existing DWARF operator and to upgrade for older styled DWARF expressions. This is done as suggested by @aprantl .

Re-based and updated.

Rebased and updated.

Update to re-base and minor correction.

Updated to re-base and included some changes separated from D70644 to keep similar changes together as suggested by @jmorse.

Updated to re-base and incorporated comments from @StephenTozer

This version is updated to rebase and to incorporate comments from @jmorse and @StephenTozer.

In D70642#1769887, @aprantl wrote:

In D70642#1769175, @alok wrote:

In D70642#1767321, @aprantl wrote:

Thanks. This part looks good, but the bitcode upgrade is still missing.

Sorry as I could not include that in first set of modifications. I was just thinking about which operations can be upgraded. One candidate for this was new expression DW_OP_LLVM_implicit_pointer which comes later that the current patch so escapes the need for upgrade. Should I look for existing operations for potential candidates for upgrade. or should i re-arrange the DW_OP_LLVM_implicit_pointer patch such as first it should not use DW_OP_LLVM_arg0 and then after current patch it needs upgrade. Could you please clear my doubt here ?

The upgrade I was thinking about here is reading old bitcode that contains

llvm.dbg.value(%foo, DILocalVariable(...), DIExpression(DW_OP_deref))

and upgrades it to

llvm.dbg.value(%foo, DILocalVariable(...), DIExpression(DW_OP_arg0, DW_OP_deref))

However, for this to work, we also need to implement support for DW_OP_argN to AsmPrinter to interpret the new operands and DIBuilder.

My assumption is that after this change DIExpression() is illegal and should be replaced by DIExpression(DW_OP_LLVM_arg0).
This transformation will cause a lot of churn in the Clang and LLVM testsuite (most of which you will be able to fix with a a sed script), but it will allow to resolve the ambiguity between:

!DIGlobalVariableExpression(!DIGlobalVariable("a"), !DIExpression())
!DIGlobalVariableExpression(!DIGlobalVariable("a"), !DIExpression(DW_OP_constu 42, DW_OP_stack_value))
!DIGlobalVariableExpression(!DIGlobalVariable("a"), !DIExpression(DW_OP_constu 42, DW_OP_plus, DW_OP_stack_value))

in the new scheme, these expressions would be

!DIExpression(DW_OP_LLVM_arg0)
!DIExpression(DW_OP_constu 42, DW_OP_stack_value) # constant, no arg!
!DIExpression(DW_OP_LLVM_arg0, DW_OP_constu 42, DW_OP_plus, DW_OP_stack_value))

respectively. We currently disambiguate between these cases by context: if the DIGlobalVariableExpression is a !dbg attachment of an llvm::Global, the global is the implicit arg0, otherwise there is no implict argument.

Since this is going to be a lot of work, we could also opt for a smaller, incremental change first that introduces only DW_OP_LLVM_arg1..DW_OP_LLVM_arg7 and keep the DW_OP_LLVM_arg0 implicit. Then we don't need to worry about bitcode upgrades now.

In D70642#1767321, @aprantl wrote:

Thanks. This part looks good, but the bitcode upgrade is still missing.

Updated to rename proposed LLVM Dwarf extension to DW_OP_LLVM_explicit_pointer.

Update includes comments from @jmorse.

This is updated to incorporate comments from @aprantl.

In D70642#1765515, @aprantl wrote:

Thanks! This will need some bitcode-compatibility planning as well:

We'll need to bump the version number, and then in MetadataLoader.cpp / upradeDIExpression() add an a DW_OP_LLVM_arg0 at the beginning for all older versions.
This should be tested with a .bc file that contains an old-form DIExpression, run through llvm-dis | llvm-as | llvm-dis and checked that the expression is upgraded correctly.

This is updated to incorporate review comments from @Orlando .

In D69999#1758776, @Orlando wrote:

Hi @alok,

Adrian said:

Thanks! Let's cycle back to this after the llvm-dev thread that @dblaikie started has been resolved.

Reading the thread I'm not sure if it counts as having been 'resolved', so hopefully these comments are not out of place.
To help keep things running smoothly please make sure:
a) variable names are capitalised, and
b) comments are written as sentences (capitals, punctuation),

Updated to incorporated review comment.

Updated to rebase due to change in parent patch D70260.

Updated to incorporate comments from @Orlando .

In D70260#1758219, @Orlando wrote:

Hi @alok. I haven't been following the DW_OP_implicit_pointer discussion too closely, so just some inline nitpick comments/questions from me.

Thanks,
Orlando

Updated to rebase due to changes in trunk and parent patches.

Updated to rebase due to changes in trunk and parent patches.

Updated to rebase due to changes in trunk and parent patches.

Updated to re-base due to changes in trunk and parent patches.

updated to re-base this due to changes in trunk and parent patches.

Updated after splitting out versions for DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_argN and llvm.dbg.derefval intrinsic.

Updated to

• Rebase
• To remove trailing space
• typecasted return value of getData to uint8_t

-Updated to use "git show" in place of "git diff" used earlier

In D69999#1743443, @alok wrote:

Review comments from @aprantl have been incorporated.

Review comments from @aprantl have been incorporated.

In D69999#1741360, @aprantl wrote:

My first thought was that this might be an opportunity to finally implement explicit dbg.value arguments. (There is prior discussion of this online somewhere but I couldn't find it in my mailbox).

The idea behind a explicit dbg.value arguments is to move away from the implicit first argument of dbg.value to a format where that makes them explicit and thus more flexible.

For example, currently we would write:

call @llvm.dbg.value(%1, !DILocalVariable(name: "x"), !DIExpression(DW_OP_uconst, 4, DW_OP_plus)

which (assuming %1 compiles to reg0) may generate something like

DW_TAG_variable
  DW_AT_name("x")
  DW_AT_location(DW_OP_reg0, DW_OP_uconst, 4, DW_OP_plus)

Note the implicit first element in the DIExpression.

An explicit notation would be something like:

call @llvm.dbg.value(%1, !DILocalVariable(name: "x"), !DIExpression(DW_OP_LLVM_arg0, DW_OP_uconst, 4, DW_OP_plus)

which has the advantage that we could put DW_OP_LLVM_arg0 into places other than just the beginning of the expression.

Similarly, we could then further extend it to allow referring to more than one LLVM SSA value within an expression, using a variadic dbg.value. For example:

call @llvm.dbg.value(%1, %2, !DILocalVariable(name: "x"), !DIExpression(DW_OP_LLVM_arg0 /* %1 */ , DW_OP_uconst, DW_OP_LLVM_arg1 /* %2 */, DW_OP_plus)

This mechanism seems to apply nicely to DW_OP_implicit_pointer — applied to your testcase we'd write:

call @llvm.dbg.value(!DILocalVariable(name: "arr"), !DILocalVariable(name: "ptr"), !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_arg0, 4)

I think this would make the handling of DW_OP_LLVM_implicit_pointer less magic in LLVM IR and would open the door to future features, such as the variadic dbg.value.
What do you think?

This patch is updated post split. part of it went to D69999, which is also the dependency for current patch. (for current patch to work, D69999 is needed)

Updated for comments from @aprantl (Adrian Prantl)

In D69999#1739404, @aprantl wrote:

• Can you update LangRef.rst / SourceLevelDebugging.rst to document the semantics of the DW_OP_implicit_pointer in LLVM IR. Specifically what the two arguments mean while in LLVM IR?

In D69886#1738047, @aprantl wrote:

Could you please split off a patch just adds the IR, Bitcode and verifier support from this review into a separate one? That one should be very straightforward to review & land and it will make the rest easier.

In D69886#1736182, @vsk wrote:

Hey @alok, thanks for working on this.