Thanks @aprantl for reviewing this. Any idea how to backport it to LLVM 11

Incorporated comment from @aprantl .

Updated to incorporate comment from @aprantl .

Updated to incorporate @aprantl 's comment.

In D87406#2266722, @rnk wrote:

I would ask you to add a test, but I can't recommend the existing test, llvm/test/DebugInfo/COFF/types-array.ll, as a good basis for a test. I'll look into rewriting it and adding coverage after you land.

@jmorse do you have any more comment on this ?

@jmorse Do you have any more comment on this ?

In D84114#2184522, @jmorse wrote:

Similar story to the parent patch, could you put a file-comment into the test explaining it's a round-trip test?

Incorporated comments from @jmorse .

Thanks for your reviewing this.

Incorporated comments from @aprantl .

In D83544#2151182, @aprantl wrote:

Mechanically, this looks great! it would be good to add a little more context to the LangRef documentation (see my inline comment).

In D80197#2059591, @thakis wrote:

This breaks check-llvm on Windows: http://45.33.8.238/win/16214/step_11.txt

Please take a look and revert if it takes a while to investigate.

In D80197#2057881, @aprantl wrote:

Thanks! I think it would be good to update clang to explicitly pass a nullptr, then we won't have as much churn in the IR for existing testcases. I think this is ready to land with that patch applied.

Incorporated comments from @aprantl and re-based.

In D80197#2056213, @aprantl wrote:

Can you confirm that this patch doesn't cause us to emit a DW_AT_lower_bound(0) where we previously didn't?

Updated the patch for comments from @aprantl .

In D80197#2051742, @aprantl wrote:

I think this is pretty good. A couple of minor comments inline.

Re-based and incorporated comments from @jmorse .

In D80197#2048855, @jmorse wrote:

I'm not incredibly familiar with bitcode and metadata, or Fortran at all, but left some comments which hopefully help. This in general looks pretty good -- do you think it's worth making Count and UpperBound mutually exclusive? Otherwise there's the chance to represent the same information twice (and thus inconsistently).

Removed -mtriple from tests.

This commit fixed clang testcases to change expected output for DISubstring which should include lowerBound=0, which is what is passed to constructor.
Below file is truncated in patch. while the test passes in my workarea. I am not sure how to include a binary file in patch.
llvm/test/Bitcode/fortranSubrangeBackward.ll.bc

Re-based and addressed comments from @aprantl

Addressed comments from @aprantl and re-based.

In D79306#2032212, @aprantl wrote:

From Paul's comment, it sounds like we do want to allow DW_OP_push_object_address in LLVM IR. Can you please add documentation for it to LangRef.rst or SourceLevelDebugging.rst (wherever the other operations are explained)?

In D79306#2027091, @aprantl wrote:

In D79306#2023359, @alok wrote:

In D79306#2020789, @aprantl wrote:

Thanks a lot for the example!

If DW_OP_push_object address is *only* going to be used inside a DISubrange and if it *always* appears at the beginning of a DIExpression bound there, we could make it implicit, the same way the first operator of a "normal" DIExpression in a dbg.value is implicitly specified by the dbg.value.

My motivation behind this is to keep the surface area of DIExpression small. We don't necessarily want to include all DWARF operators in DIExpression, because it would make it harder to emit other debug info formats from it, transform the expression during optimizations, and generally reason about them.

Would changing the backend to emit an automatic DW_OP_push_object before each DIExpression that represents a bound in a DISubrange work for flang, or are there other kinds of bound expressions / reasons why that would be a bad idea?

I understand the concerns for keeping DIExpression small. But considering the DW_OP_push_object_address implicit would prevent the freedom to use expression which don't use this (DW_OP_push_object_address) DWARF operator. If it is big a 'no' for adding DW_OP_push_object_address to DIExpression, then as you suggested I would need DW_OP_LLVM_arg0 from https://reviews.llvm.org/D70642 .

My goal is to find the most consistent and useful design for LLVM IR.

Currently (without D70642) DIExpressions modify a location specified by the context in which they appear. A DIExpression bound to a dbg.value() might modify a DW_OP_reg, a DIExpression bound by a DIGlobalVariableExpression a DW_OP_addr, etc. Based on that prior art, it would seem logical to say that a DIExpressions inside a DISubrange implicitly starts with a DW_OP_push_object_address.

Thanks for the explanation and pointers.

Incorporated comments form @aprantl. In addition to that also updated dependencies function in file DwarfCompileUnit.cpp.

In D79592#2027165, @aprantl wrote:

In D79592#2026542, @alok wrote:

In D79592#2025853, @aprantl wrote:

Why is the data location a part of the type and not something dynamically bound via dbg.value or something similar?

What's an example for what a typical data location expression?

Dynamic arrays are represented by two things

• allocated space
• descriptor which describes the array bounds (always) and allocated space (not always)

DW_AT_location attached to DW_TAG_variable denotes descriptor, which is used by bounds (using DW_OP_push_object_address). Since DW_AT_location denotes descriptor, DW_AT_data_location is used to denote allocated space. DWARF has attached that to DW_TAG_array_type.
In cases when descriptor also has a pointer to allocated space (as in case of gfortran), DW_AT_data_location can also be described by DW_OP_push_object_address and offset where the pointer to allocated space is. In cases when descriptor does not have the pointer to allocated space (as in case of flang), DW_AT_data_location can be represented by DIE reference which denotes an artificial variable which has the data pointer (dbg.declare in IR).
Please let me know if you need more information.

Thanks. I read through the DWARF v5 spec and given that arrays are represented as DICompositeTypes this makes sense to me. Personally I find the DWARF nomenclature weird (it would make more sense if we had a DW_AT_type_descriptor and a DW_AT_location), but it's probably better to stick with the DWARF naming scheme here, or else this raises even more confusion.

Thanks for your attention and comments.

Can you please add a check to Verifier.cpp that checks that only arrays get a data location?

I shall update the patch with this.

Can you please add a round-trip test to test/Assembler/ (see dicompositetype-members.ll for an example)?

I have added the round trip test named llvm/test/Bitcode/dataLocation.ll, should I move this to Assembler directory?

Can you please extend the metadata unit test llvm/unittests/IR/MetadataTest.cpp to exercise the changes in LLVMContextImpl.h?

I shall update the patch to incorporate this.

Should this be added to DIBuilder?

As of now, flang front-end doesnt utilize these routines, we can defer it or update it now itself provided f18 is now part of LLVM and later we may need.

Can you add some form of documentation that explains what the new field is used for? Perhaps in the doxygen of DIBuilder, and/or in SourceLevelDebugging.rst or LangRef.rst?

In D79592#2025853, @aprantl wrote:

Why is the data location a part of the type and not something dynamically bound via dbg.value or something similar?

What's an example for what a typical data location expression?

Incorporated comments from @vsk

In D79306#2020789, @aprantl wrote:

Thanks a lot for the example!

If DW_OP_push_object address is *only* going to be used inside a DISubrange and if it *always* appears at the beginning of a DIExpression bound there, we could make it implicit, the same way the first operator of a "normal" DIExpression in a dbg.value is implicitly specified by the dbg.value.

My motivation behind this is to keep the surface area of DIExpression small. We don't necessarily want to include all DWARF operators in DIExpression, because it would make it harder to emit other debug info formats from it, transform the expression during optimizations, and generally reason about them.

Would changing the backend to emit an automatic DW_OP_push_object before each DIExpression that represents a bound in a DISubrange work for flang, or are there other kinds of bound expressions / reasons why that would be a bad idea?

Updated for comments from @djtodoro .

update for comments from djtodoro

In D79306#2018097, @aprantl wrote:

Can you explain in more detail how flang is going to use this? In LLVM DIExpressions expressions we currently never refer to the SSA values that are bound to the expression via a dbg.value/dbg.declare from within the expression. We've had discussions about how to do this in the past (see https://reviews.llvm.org/D70642 and the discussions preceding it) and I would like to make sure that this fits well into the design.

It would be particularly helpful if you could post example LLVM IR that would be produced by flang to make use of this feature and the DWARF that should be generated from it.

In D79306#2017039, @djtodoro wrote:

I don't see you are touching the DIExpression::ExprOperand::getSize().

Can we verify the operation within DWARFVerifier ?

In D79190#2015441, @aprantl wrote:

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Updated to address comments from @probinson

In D79190#2013650, @probinson wrote:

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

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.