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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In https://reviews.llvm.org/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?


https://reviews.llvm.org/D41695