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

[sdesmalen]

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.

[sdesmalen]

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)

[dexonsmith]

In D41695#966554, @sdesmalen wrote:

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.

[probinson]

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]

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?

[aprantl]

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

[probinson]

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

[sdesmalen]

In D41695#967008, @probinson wrote:

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]

In D41695#967128, @probinson wrote:

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

[aprantl]

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.

[sdesmalen]

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]

In D41695#968478, @sdesmalen wrote:

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]

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.

[sdesmalen]

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]

In D41695#976967, @sdesmalen wrote:

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.

[sdesmalen]

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.

[aprantl]

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]

• 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]

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.

[aprantl]

In D41695#980445, @sdesmalen wrote:

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

[aprantl]

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.

[sdesmalen]

In D41695#980508, @aprantl wrote:

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

[aprantl]

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.

[sdesmalen]

In D41695#980475, @aprantl wrote:

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?

[aprantl]

In D41695#983530, @sdesmalen wrote:

In D41695#980475, @aprantl wrote:

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]

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

[sdesmalen]

In D41695#983977, @aprantl wrote:

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]

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

[sdesmalen]

In D41695#985172, @aprantl wrote:

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.

llvm/trunk/docs/LangRef.rst

 
 .. _DISubrange:
 
 DISubrange
 """"""""""
 
 ``DISubrange`` nodes are the elements for ``DW_TAG_array_type`` variants of
 :ref:`DICompositeType`. ``count: -1`` indicates an empty array.
+``count: !9`` describes the count with a :ref:`DILocalVariable`.
+``count: !11`` describes the count with a :ref:`DIGlobalVariable`.
 
 .. code-block:: llvm
 
     !0 = !DISubrange(count: 5, lowerBound: 0) ; array counting from 0
     !1 = !DISubrange(count: 5, lowerBound: 1) ; array counting from 1
     !2 = !DISubrange(count: -1) ; empty array.
 
+    ; Scopes used in rest of example
+    !6 = !DIFile(filename: "vla.c", directory: "/path/to/file")
+    !7 = distinct !DICompileUnit(language: DW_LANG_C99, ...
+    !8 = distinct !DISubprogram(name: "foo", scope: !7, file: !6, line: 5, ...
+
+    ; Use of local variable as count value
+    !9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+    !10 = !DILocalVariable(name: "count", scope: !8, file: !6, line: 42, type: !9)
+    !11 = !DISubrange(count !10, lowerBound: 0)
+
+    ; Use of global variable as count value
+    !12 = !DIGlobalVariable(name: "count", scope: !8, file: !6, line: 22, type: !9)
+    !13 = !DISubrange(count !12, lowerBound: 0)
+
 .. _DIEnumerator:
 
 DIEnumerator
 """"""""""""
 
 ``DIEnumerator`` nodes are the elements for ``DW_TAG_enumeration_type``
 variants of :ref:`DICompositeType`.
 

llvm/trunk/include/llvm/IR/DIBuilder.h

public:
     DIMacroNodeArray getOrCreateMacroArray(ArrayRef<Metadata *> Elements);
 
     /// Get a DITypeRefArray, create one if required.
     DITypeRefArray getOrCreateTypeArray(ArrayRef<Metadata *> Elements);
 
     /// Create a descriptor for a value range.  This
     /// implicitly uniques the values returned.
     DISubrange *getOrCreateSubrange(int64_t Lo, int64_t Count);
+    DISubrange *getOrCreateSubrange(int64_t Lo, Metadata *CountNode);
 
     /// Create a new descriptor for the specified variable.
     /// \param Context     Variable scope.
     /// \param Name        Name of the variable.
     /// \param LinkageName Mangled  name of the variable.
     /// \param File        File where this variable is defined.
     /// \param LineNo      Line number.
     /// \param Ty          Variable Type.

llvm/trunk/include/llvm/IR/DebugInfoMetadata.h

 
 #ifndef LLVM_IR_DEBUGINFOMETADATA_H
 #define LLVM_IR_DEBUGINFOMETADATA_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/Support/Casting.h"
 #include <cassert>
 #include <climits>
 #include <cstddef>
 #include <cstdint>
 #include <iterator>
 #include <type_traits>

 /// Array subrange.
 ///
 /// TODO: Merge into node for DW_TAG_array_type, which should have a custom
 /// type.
 class DISubrange : public DINode {
   friend class LLVMContextImpl;
   friend class MDNode;
 
-  int64_t Count;
   int64_t LowerBound;
 
-  DISubrange(LLVMContext &C, StorageType Storage, int64_t Count,
-             int64_t LowerBound)
-      : DINode(C, DISubrangeKind, Storage, dwarf::DW_TAG_subrange_type, None),
-        Count(Count), LowerBound(LowerBound) {}
+  DISubrange(LLVMContext &C, StorageType Storage, Metadata *Node,
+             int64_t LowerBound, ArrayRef<Metadata *> Ops)
+      : DINode(C, DISubrangeKind, Storage, dwarf::DW_TAG_subrange_type, Ops),
+        LowerBound(LowerBound) {}
+
   ~DISubrange() = default;
 
   static DISubrange *getImpl(LLVMContext &Context, int64_t Count,
                              int64_t LowerBound, StorageType Storage,
                              bool ShouldCreate = true);
 
+  static DISubrange *getImpl(LLVMContext &Context, Metadata *CountNode,
+                             int64_t LowerBound, StorageType Storage,
+                             bool ShouldCreate = true);
+
   TempDISubrange cloneImpl() const {
-    return getTemporary(getContext(), getCount(), getLowerBound());
+    return getTemporary(getContext(), getRawCountNode(), getLowerBound());
   }
 
 public:
   DEFINE_MDNODE_GET(DISubrange, (int64_t Count, int64_t LowerBound = 0),
                     (Count, LowerBound))
 
+  DEFINE_MDNODE_GET(DISubrange, (Metadata *CountNode, int64_t LowerBound = 0),
+                    (CountNode, LowerBound))
+
   TempDISubrange clone() const { return cloneImpl(); }
 
   int64_t getLowerBound() const { return LowerBound; }
-  int64_t getCount() const { return Count; }
+
+  Metadata *getRawCountNode() const {
+    return getOperand(0).get();
+  }
+
+  typedef PointerUnion<ConstantInt*, DIVariable*> CountType;
+
+  CountType getCount() const {
+    if (auto *MD = dyn_cast<ConstantAsMetadata>(getRawCountNode()))
+      return CountType(cast<ConstantInt>(MD->getValue()));
+
+    if (auto *DV = dyn_cast<DIVariable>(getRawCountNode()))
+      return CountType(DV);
+
+    return CountType();
+  }
 
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DISubrangeKind;
   }
 };
 
 /// Enumeration value.
 ///

llvm/trunk/lib/AsmParser/LLParser.cpp

void assign(FieldTy Val) {
     Seen = true;
     this->Val = std::move(Val);
   }
 
   explicit MDFieldImpl(FieldTy Default)
       : Val(std::move(Default)), Seen(false) {}
 };
 
+/// Structure to represent an optional metadata field that
+/// can be of either type (A or B) and encapsulates the
+/// MD<typeofA>Field and MD<typeofB>Field structs, so not
+/// to reimplement the specifics for representing each Field.
+template <class FieldTypeA, class FieldTypeB> struct MDEitherFieldImpl {
+  typedef MDEitherFieldImpl<FieldTypeA, FieldTypeB> ImplTy;
+  FieldTypeA A;
+  FieldTypeB B;
+  bool Seen;
+
+  enum {
+    IsInvalid = 0,
+    IsTypeA = 1,
+    IsTypeB = 2
+  } WhatIs;
+
+  void assign(FieldTypeA A) {
+    Seen = true;
+    this->A = std::move(A);
+    WhatIs = IsTypeA;
+  }
+
+  void assign(FieldTypeB B) {
+    Seen = true;
+    this->B = std::move(B);
+    WhatIs = IsTypeB;
+  }
+
+  explicit MDEitherFieldImpl(FieldTypeA DefaultA, FieldTypeB DefaultB)
+      : A(std::move(DefaultA)), B(std::move(DefaultB)), Seen(false),
+        WhatIs(IsInvalid) {}
+};
+
 struct MDUnsignedField : public MDFieldImpl<uint64_t> {
   uint64_t Max;
 
   MDUnsignedField(uint64_t Default = 0, uint64_t Max = UINT64_MAX)
       : ImplTy(Default), Max(Max) {}
 };
 
 struct LineField : public MDUnsignedField {
struct MDFieldList : public MDFieldImpl<SmallVector<Metadata *, 4>> {
   MDFieldList() : ImplTy(SmallVector<Metadata *, 4>()) {}
 };
 
 struct ChecksumKindField : public MDFieldImpl<DIFile::ChecksumKind> {
   ChecksumKindField() : ImplTy(DIFile::CSK_None) {}
   ChecksumKindField(DIFile::ChecksumKind CSKind) : ImplTy(CSKind) {}
 };
 
+struct MDSignedOrMDField : MDEitherFieldImpl<MDSignedField, MDField> {
+  MDSignedOrMDField(int64_t Default = 0, bool AllowNull = true)
+      : ImplTy(MDSignedField(Default), MDField(AllowNull)) {}
+
+  MDSignedOrMDField(int64_t Default, int64_t Min, int64_t Max,
+                    bool AllowNull = true)
+      : ImplTy(MDSignedField(Default, Min, Max), MDField(AllowNull)) {}
+
+  bool isMDSignedField() const { return WhatIs == IsTypeA; }
+  bool isMDField() const { return WhatIs == IsTypeB; }
+  int64_t getMDSignedValue() const {
+    assert(isMDSignedField() && "Wrong field type");
+    return A.Val;
+  }
+  Metadata *getMDFieldValue() const {
+    assert(isMDField() && "Wrong field type");
+    return B.Val;
+  }
+};
+
 } // end anonymous namespace
 
 namespace llvm {
 
 template <>
 bool LLParser::ParseMDField(LocTy Loc, StringRef Name,
                             MDUnsignedField &Result) {
   if (Lex.getKind() != lltok::APSInt || Lex.getAPSIntVal().isSigned())
bool LLParser::ParseMDField(LocTy Loc, StringRef Name, MDField &Result) {
   if (ParseMetadata(MD, nullptr))
     return true;
 
   Result.assign(MD);
   return false;
 }
 
 template <>
+bool LLParser::ParseMDField(LocTy Loc, StringRef Name,
+                            MDSignedOrMDField &Result) {
+  // Try to parse a signed int.
+  if (Lex.getKind() == lltok::APSInt) {
+    MDSignedField Res = Result.A;
+    if (!ParseMDField(Loc, Name, Res)) {
+      Result.assign(Res);
+      return false;
+    }
+    return true;
+  }
+
+  // Otherwise, try to parse as an MDField.
+  MDField Res = Result.B;
+  if (!ParseMDField(Loc, Name, Res)) {
+    Result.assign(Res);
+    return false;
+  }
+
+  return true;
+}
+
+template <>
 bool LLParser::ParseMDField(LocTy Loc, StringRef Name, MDStringField &Result) {
   LocTy ValueLoc = Lex.getLoc();
   std::string S;
   if (ParseStringConstant(S))
     return true;
 
   if (!Result.AllowEmpty && S.empty())
     return Error(ValueLoc, "'" + Name + "' cannot be empty");
#undef VISIT_MD_FIELDS
 
   Result = GET_OR_DISTINCT(GenericDINode,
                            (Context, tag.Val, header.Val, operands.Val));
   return false;
 }
 
 /// ParseDISubrange:
 ///   ::= !DISubrange(count: 30, lowerBound: 2)
+///   ::= !DISubrange(count: !node, lowerBound: 2)
 bool LLParser::ParseDISubrange(MDNode *&Result, bool IsDistinct) {
 #define VISIT_MD_FIELDS(OPTIONAL, REQUIRED)                                    \
-  REQUIRED(count, MDSignedField, (-1, -1, INT64_MAX));                         \
+  REQUIRED(count, MDSignedOrMDField, (-1, -1, INT64_MAX, false));              \
   OPTIONAL(lowerBound, MDSignedField, );
   PARSE_MD_FIELDS();
 #undef VISIT_MD_FIELDS
 
-  Result = GET_OR_DISTINCT(DISubrange, (Context, count.Val, lowerBound.Val));
+  if (count.isMDSignedField())
+    Result = GET_OR_DISTINCT(
+        DISubrange, (Context, count.getMDSignedValue(), lowerBound.Val));
+  else if (count.isMDField())
+    Result = GET_OR_DISTINCT(
+        DISubrange, (Context, count.getMDFieldValue(), lowerBound.Val));
+  else
+    return true;
+
   return false;
 }
 
 /// ParseDIEnumerator:
 ///   ::= !DIEnumerator(value: 30, name: "SomeKind")
 bool LLParser::ParseDIEnumerator(MDNode *&Result, bool IsDistinct) {
 #define VISIT_MD_FIELDS(OPTIONAL, REQUIRED)                                    \
   REQUIRED(name, MDStringField, );                                             \

llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp

for (unsigned I = 4, E = Record.size(); I != E; ++I)
       DwarfOps.push_back(getMDOrNull(Record[I]));
     MetadataList.assignValue(
         GET_OR_DISTINCT(GenericDINode, (Context, Tag, Header, DwarfOps)),
         NextMetadataNo);
     NextMetadataNo++;
     break;
   }
   case bitc::METADATA_SUBRANGE: {
-    if (Record.size() != 3)
-      return error("Invalid record");
+    Metadata *Val = nullptr;
+    // Operand 'count' is interpreted as:
+    // - Signed integer (version 0)
+    // - Metadata node  (version 1)
+    switch (Record[0] >> 1) {
+    case 0:
+      Val = GET_OR_DISTINCT(DISubrange,
+                            (Context, Record[1], unrotateSign(Record.back())));
+      break;
+    case 1:
+      Val = GET_OR_DISTINCT(DISubrange, (Context, getMDOrNull(Record[1]),
+                                         unrotateSign(Record.back())));
+      break;
+    default:
+      return error("Invalid record: Unsupported version of DISubrange");
+    }
 
-    IsDistinct = Record[0];
-    MetadataList.assignValue(
-        GET_OR_DISTINCT(DISubrange,
-                        (Context, Record[1], unrotateSign(Record[2]))),
-        NextMetadataNo);
+    MetadataList.assignValue(Val, NextMetadataNo);
+    IsDistinct = Record[0] & 1;
     NextMetadataNo++;
     break;
   }
   case bitc::METADATA_ENUMERATOR: {
     if (Record.size() != 3)
       return error("Invalid record");
 
     IsDistinct = Record[0];

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

 static uint64_t rotateSign(int64_t I) {
   uint64_t U = I;
   return I < 0 ? ~(U << 1) : U << 1;
 }
 
 void ModuleBitcodeWriter::writeDISubrange(const DISubrange *N,
                                           SmallVectorImpl<uint64_t> &Record,
                                           unsigned Abbrev) {
-  Record.push_back(N->isDistinct());
-  Record.push_back(N->getCount());
+  const uint64_t Version = 1 << 1;
+  Record.push_back((uint64_t)N->isDistinct() | Version);
+  Record.push_back(VE.getMetadataOrNullID(N->getRawCountNode()));
   Record.push_back(rotateSign(N->getLowerBound()));
 
   Stream.EmitRecord(bitc::METADATA_SUBRANGE, Record, Abbrev);
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDIEnumerator(const DIEnumerator *N,
                                             SmallVectorImpl<uint64_t> &Record,

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

TypeIndex CodeViewDebug::lowerTypeArray(const DICompositeType *Ty) {
   DINodeArray Elements = Ty->getElements();
   for (int i = Elements.size() - 1; i >= 0; --i) {
     const DINode *Element = Elements[i];
     assert(Element->getTag() == dwarf::DW_TAG_subrange_type);
 
     const DISubrange *Subrange = cast<DISubrange>(Element);
     assert(Subrange->getLowerBound() == 0 &&
            "codeview doesn't support subranges with lower bounds");
-    int64_t Count = Subrange->getCount();
+    int64_t Count = -1;
+    if (auto *CI = Subrange->getCount().dyn_cast<ConstantInt*>())
+      Count = CI->getSExtValue();
 
     // Forward declarations of arrays without a size and VLAs use a count of -1.
     // Emit a count of zero in these cases to match what MSVC does for arrays
     // without a size. MSVC doesn't support VLAs, so it's not clear what we
     // should do for them even if we could distinguish them.
     if (Count == -1)
       Count = 0;
 

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

void DwarfUnit::constructSubrangeDIE(DIE &Buffer, const DISubrange *SR,
   addDIEEntry(DW_Subrange, dwarf::DW_AT_type, *IndexTy);
 
   // The LowerBound value defines the lower bounds which is typically zero for
   // C/C++. The Count value is the number of elements.  Values are 64 bit. If
   // Count == -1 then the array is unbounded and we do not emit
   // DW_AT_lower_bound and DW_AT_count attributes.
   int64_t LowerBound = SR->getLowerBound();
   int64_t DefaultLowerBound = getDefaultLowerBound();
-  int64_t Count = SR->getCount();
+  int64_t Count = -1;
+  if (auto *CI = SR->getCount().dyn_cast<ConstantInt*>())
+    Count = CI->getSExtValue();
 
   if (DefaultLowerBound == -1 || LowerBound != DefaultLowerBound)
     addUInt(DW_Subrange, dwarf::DW_AT_lower_bound, None, LowerBound);
 
   if (Count != -1)
     // FIXME: An unbounded array should reference the expression that defines
     // the array.
     addUInt(DW_Subrange, dwarf::DW_AT_count, None, Count);

llvm/trunk/lib/IR/AsmWriter.cpp

static void writeDILocation(raw_ostream &Out, const DILocation *DL,
   Printer.printInt("line", DL->getLine(), /* ShouldSkipZero */ false);
   Printer.printInt("column", DL->getColumn());
   Printer.printMetadata("scope", DL->getRawScope(), /* ShouldSkipNull */ false);
   Printer.printMetadata("inlinedAt", DL->getRawInlinedAt());
   Out << ")";
 }
 
 static void writeDISubrange(raw_ostream &Out, const DISubrange *N,
-                            TypePrinting *, SlotTracker *, const Module *) {
+                            TypePrinting *TypePrinter, SlotTracker *Machine,
+                            const Module *Context) {
   Out << "!DISubrange(";
-  MDFieldPrinter Printer(Out);
-  Printer.printInt("count", N->getCount(), /* ShouldSkipZero */ false);
+  MDFieldPrinter Printer(Out, TypePrinter, Machine, Context);
+  if (auto *CE = N->getCount().dyn_cast<ConstantInt*>())
+    Printer.printInt("count", CE->getSExtValue(), /* ShouldSkipZero */ false);
+  else
+    Printer.printMetadata("count", N->getCount().dyn_cast<DIVariable*>(),
+                          /*ShouldSkipNull */ false);
   Printer.printInt("lowerBound", N->getLowerBound());
   Out << ")";
 }
 
 static void writeDIEnumerator(raw_ostream &Out, const DIEnumerator *N,
                               TypePrinting *, SlotTracker *, const Module *) {
   Out << "!DIEnumerator(";
   MDFieldPrinter Printer(Out);

llvm/trunk/lib/IR/DIBuilder.cpp

DITypeRefArray DIBuilder::getOrCreateTypeArray(ArrayRef<Metadata *> Elements) {
   }
   return DITypeRefArray(MDNode::get(VMContext, Elts));
 }
 
 DISubrange *DIBuilder::getOrCreateSubrange(int64_t Lo, int64_t Count) {
   return DISubrange::get(VMContext, Count, Lo);
 }
 
+DISubrange *DIBuilder::getOrCreateSubrange(int64_t Lo, Metadata *CountNode) {
+  return DISubrange::get(VMContext, CountNode, Lo);
+}
+
 static void checkGlobalVariableScope(DIScope *Context) {
 #ifndef NDEBUG
   if (auto *CT =
           dyn_cast_or_null<DICompositeType>(getNonCompileUnitScope(Context)))
     assert(CT->getIdentifier().empty() &&
            "Context of a global variable should not be a type with identifier");
 #endif
 }

llvm/trunk/lib/IR/DebugInfoMetadata.cpp

return storeImpl(new (array_lengthof(OPS)) CLASS(Context, Storage, OPS),     \
                    Storage, Context.pImpl->CLASS##s)
 #define DEFINE_GETIMPL_STORE_N(CLASS, ARGS, OPS, NUM_OPS)                      \
   return storeImpl(new (NUM_OPS)                                               \
                        CLASS(Context, Storage, UNWRAP_ARGS(ARGS), OPS),        \
                    Storage, Context.pImpl->CLASS##s)
 
 DISubrange *DISubrange::getImpl(LLVMContext &Context, int64_t Count, int64_t Lo,
                                 StorageType Storage, bool ShouldCreate) {
-  DEFINE_GETIMPL_LOOKUP(DISubrange, (Count, Lo));
-  DEFINE_GETIMPL_STORE_NO_OPS(DISubrange, (Count, Lo));
+  auto *CountNode = ConstantAsMetadata::get(
+      ConstantInt::getSigned(Type::getInt64Ty(Context), Count));
+  return getImpl(Context, CountNode, Lo, Storage, ShouldCreate);
+}
+
+DISubrange *DISubrange::getImpl(LLVMContext &Context, Metadata *CountNode,
+                                int64_t Lo, StorageType Storage,
+                                bool ShouldCreate) {
+  DEFINE_GETIMPL_LOOKUP(DISubrange, (CountNode, Lo));
+  Metadata *Ops[] = { CountNode };
+  DEFINE_GETIMPL_STORE(DISubrange, (CountNode, Lo), Ops);
 }
 
 DIEnumerator *DIEnumerator::getImpl(LLVMContext &Context, int64_t Value,
                                     MDString *Name, StorageType Storage,
                                     bool ShouldCreate) {
   assert(isCanonical(Name) && "Expected canonical MDString");
   DEFINE_GETIMPL_LOOKUP(DIEnumerator, (Value, Name));
   Metadata *Ops[] = {Name};

llvm/trunk/lib/IR/LLVMContextImpl.h

template <> struct MDNodeKeyImpl<GenericDINode> : MDNodeOpsKey {
   unsigned getHashValue() const { return hash_combine(getHash(), Tag, Header); }
 
   static unsigned calculateHash(GenericDINode *N) {
     return MDNodeOpsKey::calculateHash(N, 1);
   }
 };
 
 template <> struct MDNodeKeyImpl<DISubrange> {
-  int64_t Count;
+  Metadata *CountNode;
   int64_t LowerBound;
 
-  MDNodeKeyImpl(int64_t Count, int64_t LowerBound)
-      : Count(Count), LowerBound(LowerBound) {}
+  MDNodeKeyImpl(Metadata *CountNode, int64_t LowerBound)
+      : CountNode(CountNode), LowerBound(LowerBound) {}
   MDNodeKeyImpl(const DISubrange *N)
-      : Count(N->getCount()), LowerBound(N->getLowerBound()) {}
+      : CountNode(N->getRawCountNode()),
+        LowerBound(N->getLowerBound()) {}
 
   bool isKeyOf(const DISubrange *RHS) const {
-    return Count == RHS->getCount() && LowerBound == RHS->getLowerBound();
+    if (LowerBound != RHS->getLowerBound())
+      return false;
+
+    if (auto *RHSCount = RHS->getCount().dyn_cast<ConstantInt*>())
+      if (auto *MD = dyn_cast<ConstantAsMetadata>(CountNode))
+        if (RHSCount->getSExtValue() ==
+            cast<ConstantInt>(MD->getValue())->getSExtValue())
+          return true;
+
+    return CountNode == RHS->getRawCountNode();
   }
 
-  unsigned getHashValue() const { return hash_combine(Count, LowerBound); }
+  unsigned getHashValue() const {
+    if (auto *MD = dyn_cast<ConstantAsMetadata>(CountNode))
+      return hash_combine(cast<ConstantInt>(MD->getValue())->getSExtValue(),
+                          LowerBound);
+    return hash_combine(CountNode, LowerBound);
+  }
 };
 
 template <> struct MDNodeKeyImpl<DIEnumerator> {
   int64_t Value;
   MDString *Name;
 
   MDNodeKeyImpl(int64_t Value, MDString *Name) : Value(Value), Name(Name) {}
   MDNodeKeyImpl(const DIEnumerator *N)

llvm/trunk/lib/IR/Verifier.cpp

 
 void Verifier::visitDIScope(const DIScope &N) {
   if (auto *F = N.getRawFile())
     AssertDI(isa<DIFile>(F), "invalid file", &N, F);
 }
 
 void Verifier::visitDISubrange(const DISubrange &N) {
   AssertDI(N.getTag() == dwarf::DW_TAG_subrange_type, "invalid tag", &N);
-  AssertDI(N.getCount() >= -1, "invalid subrange count", &N);
+  auto Count = N.getCount();
+  AssertDI(Count, "Count must either be a signed constant or a DIVariable",
+           &N);
+  AssertDI(!Count.is<ConstantInt*>() ||
+               Count.get<ConstantInt*>()->getSExtValue() >= -1,
+           "invalid subrange count", &N);
 }
 
 void Verifier::visitDIEnumerator(const DIEnumerator &N) {
   AssertDI(N.getTag() == dwarf::DW_TAG_enumerator, "invalid tag", &N);
 }
 
 void Verifier::visitDIBasicType(const DIBasicType &N) {
   AssertDI(N.getTag() == dwarf::DW_TAG_base_type ||

llvm/trunk/test/Assembler/invalid-disubrange-count-node.ll

+; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
+
+; CHECK: [[@LINE+1]]:{{[0-9]+}}: error: 'count' cannot be null
+!0 = !DISubrange(count: null)

llvm/trunk/test/Bitcode/disubrange-v0.ll

+; Check that a DISubrange built with an older version of LLVM is correctly restored.
+; RUN: llvm-dis < %s.bc | FileCheck %s
+; RUN: llvm-dis < %s.bc | llvm-as | llvm-dis | FileCheck %s
+
+define void @foo(i32 %n) {
+entry:
+  %array = alloca i32, i64 42, align 16
+  call void @llvm.dbg.declare(metadata i32* %array, metadata !19, metadata !12), !dbg !18
+  ret void
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.1", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "vla.c", directory: "/path/to")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 5.0.1"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 20, type: !8, isLocal: false, isDefinition: true, scopeLine: 20, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!16, !19}
+!12 = !DIExpression()
+!16 = !DILocalVariable(name: "vla_expr", scope: !7, file: !1, line: 21, type: !17)
+!17 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!18 = !DILocation(line: 21, column: 7, scope: !7)
+!19 = !DILocalVariable(name: "array", scope: !7, file: !1, line: 21, type: !20)
+!20 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, align: 32, elements: !21)
+!21 = !{!22}
+; CHECK-DAG: ![[NODE:[0-9]+]] = !DILocalVariable(name: "array"
+; CHECK-DAG: ![[CONST:[0-9]+]] = !DISubrange(count: 42, lowerBound: 1)
+; CHECK-DAG: ![[MULTI:[0-9]+]] = !{![[CONST]]}
+; CHECK-DAG: elements: ![[MULTI]]
+!22 = !DISubrange(count: 42, lowerBound: 1)

llvm/trunk/test/Bitcode/disubrange.ll

+; Check that the DISubrange 'count' reference is correctly uniqued and restored,
+; since it can take the value of either a signed integer or a DIVariable.
+; RUN: llvm-as < %s | llvm-dis | FileCheck %s
+
+define void @foo(i32 %n) {
+entry:
+  %0 = zext i32 %n to i64
+  %vla = alloca i32, i64 %0, align 16
+  call void @llvm.dbg.declare(metadata i32* %vla, metadata !19, metadata !12), !dbg !18
+  ret void
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.1", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "vla.c", directory: "/path/to")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 5.0.1"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 20, type: !8, isLocal: false, isDefinition: true, scopeLine: 20, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!16, !19}
+!12 = !DIExpression()
+!16 = !DILocalVariable(name: "vla_expr", scope: !7, file: !1, line: 21, type: !17)
+!17 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!18 = !DILocation(line: 21, column: 7, scope: !7)
+!19 = !DILocalVariable(name: "vla", scope: !7, file: !1, line: 21, type: !20)
+!20 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, align: 32, elements: !21)
+!21 = !{!22, !23, !24, !25, !26, !27}
+
+; CHECK-DAG: ![[NODE:[0-9]+]] = !DILocalVariable(name: "vla_expr"
+; CHECK-DAG: ![[RANGE:[0-9]+]] = !DISubrange(count: ![[NODE]])
+; CHECK-DAG: ![[CONST:[0-9]+]] = !DISubrange(count: 16)
+; CHECK-DAG: ![[MULTI:[0-9]+]] = !{![[RANGE]], ![[RANGE]], ![[CONST]], ![[CONST]], ![[CONST]], ![[CONST]]}
+; CHECK-DAG: elements: ![[MULTI]]
+!22 = !DISubrange(count: !16)
+!23 = !DISubrange(count: !16)
+!24 = !DISubrange(count: 16)
+!25 = !DISubrange(count: i16 16)
+!26 = !DISubrange(count: i32 16)
+!27 = !DISubrange(count: i64 16)

llvm/trunk/test/Verifier/invalid-disubrange-count-node.ll

+; RUN: llvm-as < %s -disable-output 2>&1 | FileCheck %s
+
+define void @foo(i32 %n) {
+entry:
+  %0 = zext i32 %n to i64
+  %vla = alloca i32, i64 %0, align 16
+  call void @llvm.dbg.declare(metadata i32* %vla, metadata !19, metadata !12), !dbg !18
+  ret void
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.1", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "vla.c", directory: "/path/to")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 5.0.1"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 20, type: !8, isLocal: false, isDefinition: true, scopeLine: 20, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!16, !19}
+!12 = !DIExpression()
+!16 = !DILocalVariable(name: "vla_expr", scope: !7, file: !1, line: 21, type: !17)
+!17 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!18 = !DILocation(line: 21, column: 7, scope: !7)
+!19 = !DILocalVariable(name: "vla", scope: !7, file: !1, line: 21, type: !20)
+!20 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, align: 32, elements: !21)
+!21 = !{!22}
+; CHECK: Count must either be a signed constant or a DIVariable
+!22 = !DISubrange(count: !17)

llvm/trunk/unittests/IR/MetadataTest.cpp

TEST_F(GenericDINodeTest, getEmptyHeader) {
   EXPECT_EQ(StringRef(), N->getHeader());
   EXPECT_EQ(nullptr, N->getOperand(0));
 }
 
 typedef MetadataTest DISubrangeTest;
 
 TEST_F(DISubrangeTest, get) {
   auto *N = DISubrange::get(Context, 5, 7);
+  auto Count = N->getCount();
   EXPECT_EQ(dwarf::DW_TAG_subrange_type, N->getTag());
-  EXPECT_EQ(5, N->getCount());
+  ASSERT_TRUE(Count);
+  ASSERT_TRUE(Count.is<ConstantInt*>());
+  EXPECT_EQ(5, Count.get<ConstantInt*>()->getSExtValue());
   EXPECT_EQ(7, N->getLowerBound());
   EXPECT_EQ(N, DISubrange::get(Context, 5, 7));
   EXPECT_EQ(DISubrange::get(Context, 5, 0), DISubrange::get(Context, 5));
 
   TempDISubrange Temp = N->clone();
   EXPECT_EQ(N, MDNode::replaceWithUniqued(std::move(Temp)));
 }
 
 TEST_F(DISubrangeTest, getEmptyArray) {
   auto *N = DISubrange::get(Context, -1, 0);
+  auto Count = N->getCount();
   EXPECT_EQ(dwarf::DW_TAG_subrange_type, N->getTag());
-  EXPECT_EQ(-1, N->getCount());
+  ASSERT_TRUE(Count);
+  ASSERT_TRUE(Count.is<ConstantInt*>());
+  EXPECT_EQ(-1, Count.get<ConstantInt*>()->getSExtValue());
   EXPECT_EQ(0, N->getLowerBound());
   EXPECT_EQ(N, DISubrange::get(Context, -1, 0));
 }
 
+TEST_F(DISubrangeTest, getVariableCount) {
+  DILocalScope *Scope = getSubprogram();
+  DIFile *File = getFile();
+  DIType *Type = getDerivedType();
+  DINode::DIFlags Flags = static_cast<DINode::DIFlags>(7);
+  auto *VlaExpr = DILocalVariable::get(Context, Scope, "vla_expr", File, 8,
+                                       Type, 2, Flags, 8);
+
+  auto *N = DISubrange::get(Context, VlaExpr, 0);
+  auto Count = N->getCount();
+  ASSERT_TRUE(Count);
+  ASSERT_TRUE(Count.is<DIVariable*>());
+  EXPECT_EQ(VlaExpr, Count.get<DIVariable*>());
+  ASSERT_TRUE(isa<DIVariable>(N->getRawCountNode()));
+  EXPECT_EQ(0, N->getLowerBound());
+  EXPECT_EQ("vla_expr", Count.get<DIVariable*>()->getName());
+  EXPECT_EQ(N, DISubrange::get(Context, VlaExpr, 0));
+}
+
 typedef MetadataTest DIEnumeratorTest;
 
 TEST_F(DIEnumeratorTest, get) {
   auto *N = DIEnumerator::get(Context, 7, "name");
   EXPECT_EQ(dwarf::DW_TAG_enumerator, N->getTag());
   EXPECT_EQ(7, N->getValue());
   EXPECT_EQ("name", N->getName());
   EXPECT_EQ(N, DIEnumerator::get(Context, 7, "name"));