Page MenuHomePhabricator

[Metadata] Extend 'count' field of DISubrange to take a metadata node
ClosedPublic

Authored by sdesmalen on Jan 3 2018, 3:03 AM.

Details

Summary

This patch extends the DISubrange 'count' field to take either a
(signed) constant integer value or a reference to a DILocalVariable
or DIGlobalVariable.

This is patch [1/3] in a series to extend LLVM's DISubrange Metadata
node to support debugging of C99 variable length arrays and vectors with
runtime length like the Scalable Vector Extension for AArch64. It is
also a first step towards representing more complex cases like arrays
in Fortran.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Jan 3 2018, 3:03 AM

Not sure if this requires an RFC on llvm-dev, but please let me know if it does.
(Also feel free to add any relevant reviewers I may have missed)

fhahn added a subscriber: fhahn.Jan 3 2018, 3:09 AM

Not sure if this requires an RFC on llvm-dev, but please let me know if it does.

Given that it affects LangRef, I think it probably should get an RFC on llvm-dev so more people see it.

Should the lower bound also become an expression? My FORTRAN days are long gone but my impression is that the lower bound isn't necessarily fixed at compile-time, in more modern editions of the language.

rnk added a subscriber: rnk.Jan 3 2018, 2:38 PM

I'm concerned this will blow up memory usage of commonly encountered array types. @dexonsmith, IIRC your preferred metric for this was the private memory usage high watermark of an LTO build of clang with debug info. Do we think that's still a good proxy for a before/after comparison?

(This is partially redundant with what I said in https://reviews.llvm.org/D41697)
If your end-goal is to support a combination of DIVariables and DIExpressions in DISubrange, it might be better to extend DIExpression to support references to DIVariables and wrap the DIVariable in a DIExpression here.

In D41695#967020, @rnk wrote:

I'm concerned this will blow up memory usage of commonly encountered array types.

Which "this"? Sander's proposal looks like one node per variable-length array, which is surely not very much. I think Adrian is suggesting always having the count be an expression, which is one node per array type (with some deduplication possible, I hope, for arrays of the same length). Then I came along and suggested another one for the lower bound, but those would almost always be expressions for 0 and 1, so that's two nodes total per CU. Unless I'm misunderstanding how DIExpression works (very possible).

Should the lower bound also become an expression?

It probably should at some point, but I didn't make the change as it was not required for C99 VLAs.

rnk added a comment.Jan 4 2018, 10:58 AM
In D41695#967020, @rnk wrote:

I'm concerned this will blow up memory usage of commonly encountered array types.

Which "this"?

Turning an int64_t into a pointer to a ConstantAsMetadata of ConstantInt, which is what this does for regular arrays, I think.

Sander's proposal looks like one node per variable-length array, which is surely not very much. I think Adrian is suggesting always having the count be an expression, which is one node per array type (with some deduplication possible, I hope, for arrays of the same length). Then I came along and suggested another one for the lower bound, but those would almost always be expressions for 0 and 1, so that's two nodes total per CU. Unless I'm misunderstanding how DIExpression works (very possible).

Thanks to everyone who pointed this out. I forgot that DISubrange is storing count and lowerBound inline instead of allocating a metadata node. I'm not sure how common differently-sized array types are that this would make a noticeable difference, but we should definitely take the memory footprint serious.

Hi all, thanks for pointing out the possible memory size issue, I didn't realise that a ConstantAsMetadata was so much bigger than an 'int' and I can see how this would have an impact on memory usage. In practice it probably depends on the number of unique DISubrange nodes with non-distinct count values in the program, where I would hope that many DISubrange nodes could be merged due to similar ranges.

I ran an experiment to get high water-mark as suggested by @rnk to measure the impact, but did not see a significant difference on a Debug LTO build of Clang, but maybe I haven't run the experiment properly... I tried to monitor the cc1 and ld processes during the build and then getting the memory footprint for these processes. I'll try to run these experiments again to see if the results are sensible. Please let me know if you know of a better way to do these measurements.

aprantl added a comment.EditedJan 5 2018, 10:01 AM

Hi all, thanks for pointing out the possible memory size issue, I didn't realise that a ConstantAsMetadata was so much bigger than an 'int' and I can see how this would have an impact on memory usage. In practice it probably depends on the number of unique DISubrange nodes with non-distinct count values in the program, where I would hope that many DISubrange nodes could be merged due to similar ranges.

I ran an experiment to get high water-mark as suggested by @rnk to measure the impact, but did not see a significant difference on a Debug LTO build of Clang, but maybe I haven't run the experiment properly... I tried to monitor the cc1 and ld processes during the build and then getting the memory footprint for these processes. I'll try to run these experiments again to see if the results are sensible. Please let me know if you know of a better way to do these measurements.

I don't think LLVM uses many array types at all, so I wouldn't expect a noticeable size increase on that code base. Something array-heavy like BLAS would perhaps be a better benchmark. If memory size does turn out to be a limiting factor, we could make DISubrange variable-length.

rnk added a comment.Jan 5 2018, 10:57 AM

A good shortcut would be, rather than measuring the real high watermark memory usage, count the number of DISubranges. If it's small, then we probably don't care.

I did some further investigation on the memory size impact of having an integer 'Count' field being replaced by a ConstantAsMetadata node in DISubrange. As suggested by @rnk, I counted the number of DISubrange objects in a compilation unit for builds of Clang, BLAS, and LAPACK.

For a shared lib LTO build of Clang (for some reason I couldn't get it to work with static libs), the high-water mark of DISubrange occurrences found in one of the bigger libraries was <90. sizeof(ConstantAsMetadata) gives me 144 bytes, so this would be around 14kb of memory, which in comparison with the whole LTO Debug build feels negligable. BLAS didn't result in many DISubranges because the Fortran code uses more advanced arrays than can be described with a DISubrange using a constant count. I guess similar arguments will hold for most codes, with C/C++ using pointers in most places to 'describe' arrays. An LTO build of LAPACK resulted in a high-water mark of 45 DISubrange objects.

Perhaps I can collect more data, but I'm not so worried about replacing an integer Count with a ConstantAsMetadata node for the following reasons:

  • Similar DISubrange objects are uniqued (see test/Bitcode/disubrange.ll) and I expect many similar ranges to be merged.
  • Most array-heavy C/C++ libraries will work with pointers and malloc, and not define much 'static' arrays with constant bounds, so most types won't have their bounds described in DISubrange anyway.
  • For code or frontends that extensively use the extended 'count' field of DISubrange (as implemented in this patch), it will likely reference different Metadata nodes rather than using a constant to express some of the more advanced array types. In that case, the memory cost will be justified by the extended functionality.

The code can be changed to optimize the constant-value case, but since the impact seems small I propose to just keep it as a ConstantAsMetadata node as this keeps the code a bit simpler. Any objections to that?

rnk added a comment.Jan 16 2018, 9:20 AM

I did some further investigation on the memory size impact of having an integer 'Count' field being replaced by a ConstantAsMetadata node in DISubrange. As suggested by @rnk, I counted the number of DISubrange objects in a compilation unit for builds of Clang, BLAS, and LAPACK.

For a shared lib LTO build of Clang (for some reason I couldn't get it to work with static libs), the high-water mark of DISubrange occurrences found in one of the bigger libraries was <90. sizeof(ConstantAsMetadata) gives me 144 bytes, so this would be around 14kb of memory, which in comparison with the whole LTO Debug build feels negligable.

I'm totally satisfied with this reasoning. Thanks for checking, sorry to hold it up!

BLAS didn't result in many DISubranges because the Fortran code uses more advanced arrays than can be described with a DISubrange using a constant count. I guess similar arguments will hold for most codes, with C/C++ using pointers in most places to 'describe' arrays. An LTO build of LAPACK resulted in a high-water mark of 45 DISubrange objects.

Perhaps I can collect more data, but I'm not so worried about replacing an integer Count with a ConstantAsMetadata node for the following reasons:

  • Similar DISubrange objects are uniqued (see test/Bitcode/disubrange.ll) and I expect many similar ranges to be merged.
  • Most array-heavy C/C++ libraries will work with pointers and malloc, and not define much 'static' arrays with constant bounds, so most types won't have their bounds described in DISubrange anyway.
  • For code or frontends that extensively use the extended 'count' field of DISubrange (as implemented in this patch), it will likely reference different Metadata nodes rather than using a constant to express some of the more advanced array types. In that case, the memory cost will be justified by the extended functionality.

The code can be changed to optimize the constant-value case, but since the impact seems small I propose to just keep it as a ConstantAsMetadata node as this keeps the code a bit simpler. Any objections to that?

No, I don't think the complexity of the optimization is worth it.

Thanks @rnk for confirming and everyone for your input!

So from the feedback on this patch and specifically @aprantl's comment on my other patch (https://reviews.llvm.org/D41697#968546), along with the absense of anyone red-flagging this proposal here or on the mailing-list, I get the impression there is agreement on the intent of this patch-series to extend 'count' with references to DIVariable. In that case, I think the patch series can be subjected to further (more mechanical) code-review.

It looks like this changes the layout of DISubrange from

Flags | Count |

include/llvm/IR/DebugInfoMetadata.h
372 ↗(On Diff #128504)

Perhaps:
Optional<ConstantInt *> getConstantCount() ?

lib/Bitcode/Writer/BitcodeWriter.cpp
1445 ↗(On Diff #128504)

We typically version bitcode by adding a bit to the flags, like this:
Record.push_back(N->isDistinct() | 1<<1);
This way we don't have to waste the space for the obsoleted field.

sdesmalen updated this revision to Diff 130430.Jan 18 2018, 9:52 AM
  • Added versioning of the DISubrange node and added a corresponding unit test (.ll and .ll.bc) to test parsing of bitcode files generated from older versions of LLVM.
  • Changed 'getCount()' -> 'getConstantCountOrNull()'.
  • Changed 'getCountVariable()' -> 'getCountVariableOrNull()'
sdesmalen added inline comments.Jan 18 2018, 9:56 AM
include/llvm/IR/DebugInfoMetadata.h
372 ↗(On Diff #128504)

I changed it into getConstantCountOrNull() and getCountVariableOrNull().

lib/Bitcode/Writer/BitcodeWriter.cpp
1445 ↗(On Diff #128504)

Good suggestion, thanks! I updated the patch with the versioning and added a test-case for it as well. Let me know if you think the test-case covers it well enough.

  • Added versioning of the DISubrange node and added a corresponding unit test (.ll and .ll.bc) to test parsing of bitcode files generated from older versions of LLVM.
  • Changed 'getCount()' -> 'getConstantCountOrNull()'.
  • Changed 'getCountVariable()' -> 'getCountVariableOrNull()'

Sorry for making you do all these tiny incremental revisions, and this is clearly entering the realm of diminishing returns, but:

we could also have a single

PointerUnion<ConstantInt, DIlVariable> getCount()

method.

The fact that we are using a ConstantInt for the constants has me slightly concerned. What happens when a global points to the same ConstantInt and is optimized away? Will it also delete the ConstantInt as its last non-metadata user?

We could instead store a union { DIVariable *, int64_t }; and put a discriminator bit into the SubclassData32 or somewhere.

The fact that we are using a ConstantInt for the constants has me slightly concerned. What happens when a global points to the same ConstantInt and is optimized away? Will it also delete the ConstantInt as its last non-metadata user?

I don't think I fully understand the concern yet... I see your point about the ConstantInt itself being deleted and references to ValueAsMetadata to be removed by ValueAsMetadata::handleDeletion() (called by the destructor of Value), but Constant*'s are stored in many places in LLVM IR, so how would this use be any different?
Note that DIDerivedType does something similar for getConstant() and getStorageOffsetInBits().

The scenario I was thinking of was with the previous format used for constant DIGlobalVariables. They used to have their constant value stored as a GlobalValue and when that GlobalValue was optimized away, we also lost it in the debug info. That situation however is different than what you are doing here, you are storing a ConstantInt directly, which I presume is never deleted by any IR transformation(?), so this should be safe.

we could also have a single

PointerUnion<ConstantInt, DIlVariable> getCount()

method.

So I tried this and after seeing this change it is not that much different from having getConstantCountOrNull or getVariableCountOrNull, or even having a getRawCountNode() and using a dyn_cast<> to get the results. Most accesses to getCount would look like:

if (auto *CI = SR->getCount().dyn_cast<ConstantInt*>()) ...

The benefit of using PointerUnion is that we know the result of getCount() is either a ConstantInt, a DIVariable (or is invalid), but the same could be argued for getConstantCountOrNull() and getVariableCountOrNull(). The downside of using PointerUnion is that we can only support two kinds of count types, and not something like DIExpression or something else if that is ever needed in the future (although specifically for DIExpression we agreed this wasn't necessary for the purposes I had in mind, but I think you see my point).

I would be tempted to stick with the current interface, but let me know if you want me to post the patch(es) that use PointerUnion.

The scenario I was thinking of was with the previous format used for constant DIGlobalVariables. They used to have their constant value stored as a GlobalValue and when that GlobalValue was optimized away, we also lost it in the debug info. That situation however is different than what you are doing here, you are storing a ConstantInt directly, which I presume is never deleted by any IR transformation(?), so this should be safe.

I read this as 'don't need to make changes', is that correct?

we could also have a single

PointerUnion<ConstantInt, DIlVariable> getCount()

method.

So I tried this and after seeing this change it is not that much different from having getConstantCountOrNull or getVariableCountOrNull, or even having a getRawCountNode() and using a dyn_cast<> to get the results. Most accesses to getCount would look like:

if (auto *CI = SR->getCount().dyn_cast<ConstantInt*>()) ...

The benefit of using PointerUnion is that we know the result of getCount() is either a ConstantInt, a DIVariable (or is invalid), but the same could be argued for getConstantCountOrNull() and getVariableCountOrNull().

With two ...OrNull() methods the interface suggests that both of them could be null, or both of them could be nonnull. With the PointerUnion it unambiguously encodes that this is an either or situation (though it still allows the both are null case, which we also don't need).

The downside of using PointerUnion is that we can only support two kinds of count types, and not something like DIExpression or something else if that is ever needed in the future (although specifically for DIExpression we agreed this wasn't necessary for the purposes I had in mind, but I think you see my point).

Not really :-)
PointerUnion.h also defines a PointerUnion3 and PointerUnion4.

I would be tempted to stick with the current interface, but let me know if you want me to post the patch(es) that use PointerUnion.

Personally I think that the PointerUnion interface is nicer, but it's arguably not very important.

The scenario I was thinking of was with the previous format used for constant DIGlobalVariables. They used to have their constant value stored as a GlobalValue and when that GlobalValue was optimized away, we also lost it in the debug info. That situation however is different than what you are doing here, you are storing a ConstantInt directly, which I presume is never deleted by any IR transformation(?), so this should be safe.

I read this as 'don't need to make changes', is that correct?

Correct, thanks.

sdesmalen updated this revision to Diff 131061.Jan 23 2018, 7:06 AM

Changed interface for getConstantCountOrNull() and getVariableCountOrNull() to PointerUnion<ConstantInt*, DIVariable*> getCount()

The downside of using PointerUnion is that we can only support two kinds of count types, and not something like DIExpression or something else if that is ever needed in the future (although specifically for DIExpression we agreed this wasn't necessary for the purposes I had in mind, but I think you see my point).

Not really :-)
PointerUnion.h also defines a PointerUnion3 and PointerUnion4.

Ah I didn't know about these, thanks for pointing out!

With two ...OrNull() methods the interface suggests that both of them could be null, or both of them could be nonnull. With the PointerUnion it unambiguously encodes that this is an either or situation (though it still allows the both are null case, which we also don't need).

Right, I see your point about the distinction, I have updated the patches to use PointerUnion instead.

aprantl accepted this revision.Jan 23 2018, 8:23 AM

I have a few more comments inline, with the exception of the one in MetadataLoader, all just minor nitpicks.

LGTM with those changes addressed!
Thanks!

lib/AsmParser/LLParser.cpp
3892 ↗(On Diff #131061)

missing . at the end

3902 ↗(On Diff #131061)

same here.

4067 ↗(On Diff #131061)

the application of { is inconsistent in this nested if-statements.

lib/Bitcode/Reader/MetadataLoader.cpp
1178 ↗(On Diff #131061)

Please add a comment here explaining the difference between version 0 and version 0.

1188 ↗(On Diff #131061)

This should be return error("...")

This revision is now accepted and ready to land.Jan 23 2018, 8:23 AM
sdesmalen marked 5 inline comments as done.Jan 24 2018, 1:58 AM

I have a few more comments inline, with the exception of the one in MetadataLoader, all just minor nitpicks.

LGTM with those changes addressed!
Thanks!

Great, thanks for your review and helping to get these patches in! I have addressed all your comments in the commit.

This revision was automatically updated to reflect the committed changes.