Page MenuHomePhabricator

[DebugInfo] Support for DW_TAG_generic_subrange
ClosedPublic

Authored by alok on Oct 11 2020, 11:30 PM.

Details

Summary
  This is needed to support fortran assumed rank arrays which
  have runtime rank.

Summary:

Fortran assumed rank arrays have dynamic rank. DWARF TAG
DW_TAG_generic_subrange is needed to support that.

Testing:

unit test cases added (hand-written)
check llvm
check debug-info

Diff Detail

Event Timeline

alok created this revision.Oct 11 2020, 11:30 PM
Herald added a project: Restricted Project. · View Herald Transcript
alok requested review of this revision.Oct 11 2020, 11:30 PM
djtodoro added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
392

using instead?

alok marked an inline comment as done.Oct 12 2020, 1:54 AM
alok added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
392

Thanks for your comment. I shall update patch for this.

alok updated this revision to Diff 297515.Oct 12 2020, 2:08 AM
alok marked an inline comment as done.

Updated with comment from @djtodoro

SouraVX added inline comments.
llvm/test/DebugInfo/X86/dwarfdump-generic_subrange_count.ll
4

NIT: One liner will suffice ?

; RUN: llc %s -filetype=obj - o - | llvm-dwarfdump - | FileCheck %s
alok marked an inline comment as done.Oct 12 2020, 9:12 AM
alok added inline comments.
llvm/test/DebugInfo/X86/dwarfdump-generic_subrange_count.ll
4

Thanks for your comment. Next version of patch would include it.

aprantl added inline comments.Oct 12 2020, 11:23 AM
llvm/lib/IR/LLVMContextImpl.h
369

Why is Node1 == Node2 not enough here?

alok updated this revision to Diff 297772.Oct 12 2020, 11:46 PM
alok marked an inline comment as done.

Updated to re-base, incorporate comment from @SouraVX and changes to pacify pre-built check which expects function names to start with small letter and Variable with capital letter, Newer added function gets created with macro. Changed macro and consequently had to change existing functions.

Some nits included.

llvm/include/llvm/IR/DebugInfoMetadata.h
385

I think we can remove this newlines.

395

Here as well.

llvm/lib/AsmParser/LLParser.cpp
4685

Don't need to initialize this to nullptr since the convToMetadata() will do that for us?
Therefore, we can just do:

Metadata *Count = convToMetadata(count);
..
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1456

We don't need these extra newlines.

llvm/lib/IR/LLVMContextImpl.h
386

I suggest:

auto *MD = dyn_cast<ConstantAsMetadata>(CountNode);
if (CountNode && MD)
  return hash_combine(cast<ConstantInt>(...);
return hash_combine(CountNode,...);
djtodoro added inline comments.Oct 12 2020, 11:53 PM
llvm/lib/AsmParser/LLParser.h
181 ↗(On Diff #297772)

This should be a separate (NFC) patch. What is the motivation for this? Does this follow the https://llvm.org/docs/ProgrammersManual.html?

alok added inline comments.Oct 12 2020, 11:59 PM
llvm/lib/AsmParser/LLParser.h
181 ↗(On Diff #297772)

Actually previous patch made pre-built check unhappy.
https://s3.amazonaws.com/buildkiteartifacts.com/1adfd68e-67d4-4646-be3f-d34c6750f042/f8ab115f-a384-49e8-a048-0f71ab03c5d0/9a2d3de1-03f8-4aa5-bf73-0649ac80bd99/cb7e0517-ba2c-4279-b879-946045e60937/clang-tidy.txt?response-content-type=text%2Fplain&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAQPCP3C7L5KVWOABY%2F20201012%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20201012T125417Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEFIaCXVzLWVhc3QtMSJIMEYCIQDc8ieKOzWDEk%2BL0BPwnVZomYsevkqGvyyQf5YvvuWbTwIhAMe3ThgzKm1RHgbJjTu1YP2XpcN85puGzAEQBjYRbrckKr0DCIv%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEQABoMMDMyMzc5NzA1MzAzIgxAEYpGgHkNuGgl8RYqkQMCl7aH3Nb0QpQNi9tvShyV7AhQ0PbgaAGBTKy4LqSfNQ%2Bg7fGW71rAZhte%2Bfi2IYRf0cD9kdM1OSqaVdoY7BHHdWbhP5Rox6pz2yz6Wra9ffz44bfiEyFT6x%2Fywt7%2FSUuFxwf%2F59wc%2FiGK5Uq8%2B2L9wNAxGhQNNpBkizvzRyte7ZXpss92CGuaL8lwbPI1Om4zFBTy040DPOEiL6%2FmO%2BWDtHynPHDUTRbc%2BiX4rw%2BLiMJ%2F8C3g5xHGEjfGIxeQ4tqwh2JyRtMSeTvx4HaGyIxHBCDSSGr6hly3Lk%2Bg2HNYdd6FurldOLDCLYGCSs%2BKAOD2kKa3IZd%2BOBQyBAsle2%2F2YMYuKE02jeSMHg3Ps0WKLmbOqxtXuC6bid0SFapuu2AqTMDZvYDfON%2BXwb4KqYrr%2FjLWdBi%2Bf72SkCvDdrcwTrgycKa%2B6EHjxGEwAoIHIzVcKNPKaJU8tjW1uBKkVqUEZAhWaf4flfdU7jKgGEK2ahFoiMAb0cJ%2Fnhst9KPj26PdQQBAripVTvc%2B4%2FdUrlWEyDDy05D8BTrqASQWXXSDVn4f6gX%2BrKEUBeZM%2BMU55XdjESpYDAMiFCG3NBjl6T7yrImTJRkSGqtWBuqB5i3lhn7M8vLLINmvIy5BVjPS%2BpQbmFqh2d%2Blx2nkZ4EiAcmE5Sf%2F6xkb58%2B%2FqgY5FyyQeH6jm8bni7AdCOIOexw6JCUrG9zGE87MMSDG0jzmougkSmLm1T4%2F1ubu69YjAAFJrO3RkReEM1DpZ96hyMjnOJ8XtUZdGawrLp5IwtBBfR0rEjgd2ZdsRyo8CoLEkPvyOTlJC7f%2B4%2F%2FCVU%2BJETObP4L6zEXCpDP39Tl8Mqw2jeL6UnWeEw%3D%3D&X-Amz-Signature=dad707a9581c146c88549356acc2b69738ab9efb51d4fe385ec556c3a6084b68

To correct that, changing a macro was needed which led change to other parse functions (to lower case the first letter of function). Along with that I changed few other functions I came across.
Your comment for separate patch if only related to "error" or in general ?

djtodoro added inline comments.Oct 13 2020, 12:05 AM
llvm/lib/AsmParser/LLParser.h
181 ↗(On Diff #297772)

I mean in general, any refactoring/renaming should go as a separate (NFC) patch, since the change with that included, becomes very large (and hard for reviewing).

I cannot see the content from the link, but I think you can ignore the clang-format warnings for now, since it isn't related to this change I guess.

alok added inline comments.Oct 13 2020, 1:06 AM
llvm/lib/AsmParser/LLParser.h
181 ↗(On Diff #297772)

Thanks for your suggestion. I shall remove these changes from current patch.

alok marked 4 inline comments as done.Oct 13 2020, 1:55 AM
alok added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
385

Thanks. I shall include this in next revision of patch.

395

Thanks. I shall include this in next revision of patch.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1456

Thanks. I shall include this in next revision of patch.

llvm/lib/IR/LLVMContextImpl.h
386

Thanks. I shall include this in next revision of patch.

alok updated this revision to Diff 297794.Oct 13 2020, 2:14 AM
alok marked 4 inline comments as done.

Incorporated comments from @djtodoro and also removed function re-naming due to pre-built clang-tidy warnings.
Now all other format warning except first are corrected.


/mnt/disks/ssd0/agent/llvm-project/llvm/lib/AsmParser/LLParser.cpp:4599:16: warning: invalid case style for function 'ParseDIGenericSubrange' [readability-identifier-naming]
bool LLParser::ParseDIGenericSubrange(MDNode *&Result, bool IsDistinct) {

^~~~~~~~~~~~~~~~~~~~~~
parseDiGenericSubrange

/mnt/disks/ssd0/agent/llvm-project/llvm/lib/AsmParser/LLParser.cpp:4613:8: warning: invalid case style for variable 'convToMetadata' [readability-identifier-naming]

auto convToMetadata = [&](MDSignedOrMDField Bound) -> Metadata * {
     ^~~~~~~~~~~~~~
     ConvToMetadata

/mnt/disks/ssd0/agent/llvm-project/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1394:8: warning: invalid case style for variable 'DW_generic_Subrange' [readability-identifier-naming]

DIE &DW_generic_Subrange =
     ^~~~~~~~~~~~~~~~~~~
     DwGenericSubrange

/mnt/disks/ssd0/agent/llvm-project/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1400:8: warning: invalid case style for variable 'addBoundTypeEntry' [readability-identifier-naming]

auto addBoundTypeEntry = [&](dwarf::Attribute Attr,
     ^~~~~~~~~~~~~~~~~
     AddBoundTypeEntry

alok marked an inline comment as done.Oct 13 2020, 4:59 AM
alok added inline comments.
llvm/lib/IR/LLVMContextImpl.h
369

This is to cover cases arising due to below getters of ConstantInt.


ConstantInt *getInt8(uint8_t C) {
  return ConstantInt::get(getInt8Ty(), C);
}

/// Get a constant 16-bit value.
ConstantInt *getInt16(uint16_t C) {
  return ConstantInt::get(getInt16Ty(), C);
}

/// Get a constant 32-bit value.
ConstantInt *getInt32(uint32_t C) {
  return ConstantInt::get(getInt32Ty(), C);
}

/// Get a constant 64-bit value.
ConstantInt *getInt64(uint64_t C) {
  return ConstantInt::get(getInt64Ty(), C);
}

For the same value say C=1, different getIntX gives different Nodes. So it looks better to compare values and ignoring size/type.

aprantl added a comment.EditedOct 16 2020, 1:35 PM

Cab you remind me why we are allowing ConstantAsMetadata nodes at all instead of allowing only DIExpressions?

alok marked an inline comment as done.Oct 20 2020, 11:19 PM

Cab you remind me why we are allowing ConstantAsMetadata nodes at all instead of allowing only DIExpressions?

Assumed rank is relatively new as well as advance feature in Fortran. It looks like DIExpression should be enough currently with the current understanding. One way is to start with DIExpression only and keep adding other types as and when needed (with addition to feature), but I kept it generalized to keep the representation of bounds rich as well as similar to usual array bounds (with permissible types).

alok updated this revision to Diff 300967.Oct 27 2020, 5:57 AM

Addressed concern of @aprantl by getting rid of ConstantAsMetadata and using DIExpression to represent integer type of bounds.

aprantl added inline comments.Oct 27 2020, 6:47 PM
llvm/include/llvm/IR/DIBuilder.h
587

so this can be a more specific type now?

Also, I'm sorry for being so slow at reviewing these days...

alok updated this revision to Diff 301283.Oct 28 2020, 7:41 AM

Addressed comments from @aprantl to specialize DIBuilder routine arguments and added tests.

aprantl accepted this revision.Oct 28 2020, 9:24 AM
This revision is now accepted and ready to land.Oct 28 2020, 9:24 AM
This revision now requires review to proceed.Oct 28 2020, 9:24 AM
dexonsmith resigned from this revision.Oct 28 2020, 12:29 PM

I had a misconfigured Herald rule; I should have been added as a subscriber not a blocking reviewer.

This revision is now accepted and ready to land.Oct 28 2020, 12:29 PM
This revision was landed with ongoing or failed builds.Oct 28 2020, 1:09 PM
This revision was automatically updated to reflect the committed changes.
markus added a subscriber: markus.Oct 29 2020, 2:50 AM
markus added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
546

Question: aren't we missing a emitOp(dwarf::DW_OP_consts) here? AFAICT emitSigned is not similar to emitConstu in that sense (i.e. the former does not do emitOp internally).

alok added inline comments.Oct 29 2020, 3:17 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
546

Thanks for pointing this out. You are correct this is missed. I plan to commit the correction. I hope this would not require separate review ?

alok added inline comments.Oct 29 2020, 3:51 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
546

This is corrected in commit ID: 930a8c60b60805567e3cc0c7958be3ceeafd01f9
Thanks again for pointing this out.

dstenb added a subscriber: dstenb.Oct 29 2020, 3:55 AM
dstenb added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
546

Okay, thanks! It could probably be good to also add a small regression test using llvm-dwarfdump to verify that the operation is emitted correctly.

alok marked an inline comment as done.Oct 29 2020, 5:39 AM
alok added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
546

Thanks for your suggestion. It is included in commit ID: aa71874f6b9bb9bddca54e9d89d5725dcc77090f