Page MenuHomePhabricator

[DebugInfo] Upgrade DISubrange to support Fortran dynamic arrays
ClosedPublic

Authored by alok on Tue, May 19, 2:04 AM.

Details

Summary
Summary:

This patch upgrades DISubrange to support fortran requirements.
Below are the updates/addition of fields.
lowerBound - Now accepts signed integer or DIVariable or DIExpression, earlier it accepted only signed integer.
upperBound - This field is now added and accepts signed interger or DIVariable or DIExpression.
stride - This field is now added and accepts signed interger or DIVariable or DIExpression.

Earlier, count was REQUIERED field, now either of count or upperBound
must be present.
Earlier, absence of lowerBound was considered as lowerBound=0 (considering only c),
considering Fortran which has default lowerBound=1, we'll not stick to that
assumption, Now distinct node will be created for absent lowerBound.

This is required to describe bounds of array which are known at runtime.

Testing:
  • added unit tests (hand written)
  • check clang
  • check llvm
  • check debug-info

Diff Detail

Event Timeline

alok created this revision.Tue, May 19, 2:04 AM
alok updated this revision to Diff 264859.Tue, May 19, 4:57 AM

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

alok edited the summary of this revision. (Show Details)Tue, May 19, 4:58 AM
alok updated this revision to Diff 264864.Tue, May 19, 5:16 AM

Removed -mtriple from tests.

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).

There's a reasonable amount of code (DWARF printing for example) that's repeated, IMO it would be worth using lambdas more often to ease readability.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1260–1265

This comment will want updating for version two.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1530

Could I suggest 2 << 1 instead? From my very limited understanding, the Distinct field goes in the bottom bit, and writing this as 2 << 1 makes it clear "we're shifting the version up one bit", wheras 1 << 2 makes it feel like the number is a flag.

(This may be a style thing, and thus unimportant).

llvm/lib/IR/AsmWriter.cpp
1870–1875

As far as I can tell, printMetadata isn't overloaded, so you should be able to pass the non-null un-casted result of getLowerBound to printMetadata, without having to examine its type further.

llvm/lib/IR/LLVMContextImpl.h
370–371

Unless I'm reading this wrong, when:

  • RHSStrideis a ConstantInt,
  • Strideis something else

The first if will be true, the second if false, and we won't return false here despite the two things being different.

That's probably fixed by combining the first two ifs into one larger precondition ("if both are CosntantInts..").

llvm/test/Bitcode/fortranSubrange.ll
7

In here (and other tests) you probably want to capture the upperBound metadata node number and check that it's referring to the correct node too. !4 would be a legitimate node here, but as empty metadata, wouldn't be correct.

alok marked 10 inline comments as done.Fri, May 22, 11:14 AM

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).

Thanks for all your valuable comments. Yes, I shall update the patch for this.

There's a reasonable amount of code (DWARF printing for example) that's repeated, IMO it would be worth using lambdas more often to ease readability.

Thanks for your suggestion. I shall incorporate this in next version.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1260–1265

Thanks for pointing this out. It'll be updated in next version.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1530

Yes, true. Makes sense. It'll be updated in next version.

llvm/lib/IR/AsmWriter.cpp
1870–1875

Thanks for pointing this out. It'll be updated in next version.

llvm/lib/IR/LLVMContextImpl.h
370–371

Yes, correct. It'll be updated in next version.

llvm/test/Bitcode/fortranSubrange.ll
7

Yes, thanks for your comment. It'll be updated in next version.

alok marked 5 inline comments as done.Fri, May 22, 12:48 PM
alok updated this revision to Diff 265789.Fri, May 22, 12:54 PM

Re-based and incorporated comments from @jmorse .

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

llvm/include/llvm/IR/DIBuilder.h
577

Why does this take a Metadata * over a DIExpression * or something else more specific?
Update: The PointerUnion<> that is BoundType?

llvm/include/llvm/IR/DebugInfoMetadata.h
350

I think it might be reasonable to assert() that the type is well-formed here.

359

Can you move these function bodies into the .cpp file?

llvm/lib/IR/Verifier.cpp
913

In other DINodes we have getSomeAttribute() method assert when it's malformed, but the getRawAttribute() returns a raw Metadata pointer, and the Verifier checks that the Metadata node is a legal one.

llvm/unittests/IR/MetadataTest.cpp
1269

Let me know if I missed it: Is there a test for the LLVMContextImpl hashing here? Where it's tested that two DISubranges that differ in only one attribute compare differently?

alok marked 10 inline comments as done.Sat, May 23, 1:06 PM

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

Thanks a lot for your comments. I shall update the patch in next version.

llvm/include/llvm/IR/DIBuilder.h
577

Yes, that is an option. PointerUnion and Metadata both can represent signed int (ConstantAsMetadata), DIVariable and DIExpression. PointerUnion is more specific as you mentioned, as other places will reject what extra types Metadata can have. I just followed the way Count is handled which also has PointerUnion type.

llvm/include/llvm/IR/DebugInfoMetadata.h
350

Sure, I shall add that in next version of patch.

359

Sure. It will be updated in next version of patch.

llvm/lib/IR/Verifier.cpp
913

Thanks for your comment. It will be included in next version of patch.

llvm/unittests/IR/MetadataTest.cpp
1269

Thanks for pointing this out. It will be included in next version of patch.

alok marked 5 inline comments as done.Sat, May 23, 1:07 PM
alok updated this revision to Diff 265884.Sat, May 23, 1:13 PM

Updated the patch for comments from @aprantl .

aprantl added inline comments.Tue, May 26, 4:59 PM
clang/test/CodeGen/debug-info-matrix-types.c
14 ↗(On Diff #265884)

Should we just make lowerBound implicit and only print it when it is non-null?

llvm/lib/IR/AsmWriter.cpp
1866

calculated

I think this comment is wrong, since it doesn't account for stride.

What about: A lowerBound of constant 0 should not be skipped, since it is different from an unspecified lower bound (= nullptr).

Similarly, should the existing use-cases in the test suite be nullptr values or constant zero?

llvm/unittests/IR/MetadataTest.cpp
1266

Should we also test that LV/UV of 0 is distinct from nullptr?

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

alok marked 6 inline comments as done.Wed, May 27, 9:59 AM

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

Yes, it would not emit DW_AT_lower_bound(0) for C and it would also not emit DW_AT_lower_bound(1) for fortran and similarly for other language their defaults. I'll add test case for this.

clang/test/CodeGen/debug-info-matrix-types.c
14 ↗(On Diff #265884)

Currently clang front-end is using old constructor for DISubrange which accepts lower as int, while using old constructor it passes zero (0) for lowerBound so it is explicit. Using new constructor and passing nullptr for lowerBound would make it not to be printed.

llvm/lib/IR/AsmWriter.cpp
1866

Thanks for pointing it out and suggesting replacement. It'll be incorporated.

llvm/unittests/IR/MetadataTest.cpp
1266

Sure, it'll be added in next version of patch.

alok marked 3 inline comments as done.Wed, May 27, 10:00 AM
alok updated this revision to Diff 266597.Wed, May 27, 10:34 AM

Incorporated comments from @aprantl and re-based.

aprantl accepted this revision.Wed, May 27, 11:09 AM

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.

clang/test/CodeGen/debug-info-matrix-types.c
14 ↗(On Diff #265884)

Would you mind preparing a patch for clang to use the new constructor? Then we wouldn't need to update all these testcases.

This revision is now accepted and ready to land.Wed, May 27, 11:09 AM
alok added a comment.Thu, May 28, 1:19 AM

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.

Thanks for review. The required patch is incorporated.

alok marked 2 inline comments as done.Thu, May 28, 1:20 AM
alok added inline comments.
clang/test/CodeGen/debug-info-matrix-types.c
14 ↗(On Diff #265884)

Thanks. Patch is updated as required.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 28, 1:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

alok marked an inline comment as done.Thu, May 28, 5:44 AM

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.

Fixed in 7716681cfd0ea2dadbddae6f1983e130c2fa4247 .

melver added a subscriber: melver.Thu, May 28, 5:50 AM
melver added inline comments.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1522

rotateSign() is no longer used in this file. If there are no plans to use it again, please remove it.
Thanks!

alok marked 2 inline comments as done.Thu, May 28, 6:44 AM
alok added inline comments.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1522
alok marked an inline comment as done.Thu, May 28, 10:10 PM