Page MenuHomePhabricator

[DebugInfo][flang]Added support for representing Fortran assumed length strings
ClosedPublic

Authored by SouraVX on Aug 20 2020, 9:53 AM.

Details

Summary

This patch adds support for representing Fortran character(n).

Primarily patch is based out of D54114 with appropriate modifications.

Test case IR is generated using our downstream classic-flang. We're in process
of upstreaming flang PR's but classic-flang has dependencies on llvm, so
this has to get in first.

Patch includes functional test case for both IR and corresponding
dwarf, furthermore it has been manually tested as well using GDB.

Source snippet:

program assumedLength
  call sub('Hello')
  call sub('Goodbye')
  contains
  subroutine sub(string)
          implicit none
          character(len=*), intent(in) :: string
          print *, string
  end subroutine sub
end program assumedLength

GDB:

(gdb) ptype string
type = character (5)
(gdb) p string
$1 = 'Hello'

Co-authored By: Erich Schweitz <eschweitz@nvidia.com>, Bhuvanendra Kumar bhuvanendra.kumarn@amd.com>

Diff Detail

Event Timeline

SouraVX created this revision.Aug 20 2020, 9:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
SouraVX requested review of this revision.Aug 20 2020, 9:53 AM
SouraVX added inline comments.Aug 20 2020, 9:56 AM
llvm/docs/SourceLevelDebugging.rst
1065

I did not know much about by Doxygen, is the missing new-line will cause failure ?

Thanks, this looks good to me — I just have a few inline comments.

llvm/docs/SourceLevelDebugging.rst
1057

DWARF tags

1069

same here

llvm/include/llvm/IR/DebugInfoMetadata.h
852

Are these copy&paste errors?

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

// For eventual Unicode support.

llvm/lib/IR/LLVMContextImpl.h
429

My guess is that we can get away with only hashing Tag+Name, or Tag+Size+Encoding, but I'll leave that up to you.

SouraVX added inline comments.Aug 20 2020, 11:09 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
852

Nope, just for backward compatilbility I guess ? just like other nodes. Notice one expects MDString and other one StringRef

aprantl added inline comments.Aug 20 2020, 12:37 PM
llvm/include/llvm/IR/DebugInfoMetadata.h
852

I see. But since this is a new node, we don't need all the variants with the default arguments, do we? I think I'd be more comfortable with the minimal set we can get away with.

SouraVX updated this revision to Diff 286891.Aug 20 2020, 1:21 PM

Addressed @aprantl review comments. Thanks for reviewing this!
Changes:

  • Reduced to minimal variants.
  • And other Nit comments.
SouraVX marked 4 inline comments as done.Aug 20 2020, 1:25 PM
SouraVX added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
852

This looks reasonable ?

llvm/lib/IR/LLVMContextImpl.h
429

Trimmed down to Tag + Name + Encoding.
Thanks for pointing this out!

aprantl accepted this revision.Aug 20 2020, 1:27 PM
This revision is now accepted and ready to land.Aug 20 2020, 1:27 PM
SouraVX marked an inline comment as done.Aug 20 2020, 1:31 PM

Thanks @aprantl for reviewing this. Let's wait for @schweitz also to have a look. Planning to land this tomorrow or so.

schweitz accepted this revision.Aug 20 2020, 2:38 PM
schweitz added inline comments.
llvm/include/llvm/Bitcode/LLVMBitCodes.h
342

Wasn't this gap in the numbering because METADATA_STRING_TYPE originally came before METADATA_COMMON_BLOCK in my original changes?

SouraVX added inline comments.Aug 20 2020, 10:43 PM
llvm/include/llvm/Bitcode/LLVMBitCodes.h
342

For the record yes! Looking at the history, GAP 40 - 44 was introduced in when COOMMON BLOCK was introduced in D54327.
It shouldn't cause an issue. But for uniformities sake should we make it 41 ? this will reduce the GAP and we can follow this for upcoming in this direction.
@aprantl you want to add on this ?

SouraVX updated this revision to Diff 287033.EditedAug 21 2020, 7:39 AM

Updated BitCodes for DIString to 41, this will narrow down the gap. Moving forward when introducing new metadata we can use 42-43 bits to compensate the gap.

schweitz accepted this revision.Aug 21 2020, 9:06 AM

Thanks, Sourabh. That will help sync these patches with our internal branch. Glad to see this get upstreamed.

You might want to add "[flang]" to the subject line as this change supports Fortran. Thanx.

SouraVX retitled this revision from [DebugInfo]Added support for representing Fortran assumed length strings to [DebugInfo][flang]Added support for representing Fortran assumed length strings.Aug 21 2020, 9:11 AM

You might want to add "[flang]" to the subject line as this change supports Fortran. Thanx.

Done!, will wait for @aprantl to have another look, before pushing.

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

SouraVX added a comment.EditedAug 21 2020, 10:09 AM

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

I'll have to re-confirm with our team. Making sure we don't break anything is priority for us.
@schweitz do you have any reservations WRT this ?

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

I'll have to re-confirm with our team. Making sure we don't break anything is priority for us.
@schweitz do you have any reservations WRT this ?

We don't have any reservations with bit 42 and 43. If @schweitz also don't have any reservations we can mark/document them to be usable by others.

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

I'll have to re-confirm with our team. Making sure we don't break anything is priority for us.
@schweitz do you have any reservations WRT this ?

We don't have any reservations with bit 42 and 43. If @schweitz also don't have any reservations we can mark/document them to be usable by others.

42 and 43 are being used on our branch to support Fortran array debugging.

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

I'll have to re-confirm with our team. Making sure we don't break anything is priority for us.
@schweitz do you have any reservations WRT this ?

Sounds good to me.

SouraVX updated this revision to Diff 287095.Aug 21 2020, 1:24 PM

Added comments for reserved bit codes.

This revision was landed with ongoing or failed builds.Aug 21 2020, 9:44 PM
This revision was automatically updated to reflect the committed changes.
SouraVX added a comment.EditedAug 21 2020, 9:50 PM

I don't know what's the criteria for arc land for removing strings from commit message. I added 2 Co-author: Eric Schweitz and Bhuvanendra kumar(from AMD). But apparently it dropped it when landing the actual patch. :(
Are you guys Okay with this ?
Anyway, I'll modify the summary here as well(for attributing co-author's).

SouraVX edited the summary of this revision. (Show Details)Aug 21 2020, 9:52 PM
SouraVX edited the summary of this revision. (Show Details)Aug 21 2020, 9:54 PM