[PATCH] IR: Add specialized debug info metadata nodes

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Feb 4 16:44:59 PST 2015


> On 2015-Feb-04, at 16:17, Adrian Prantl <aprantl at apple.com> wrote:
> 
> 
>> On Feb 4, 2015, at 3:53 PM, Frédéric Riss <friss at apple.com> wrote:
>> 
>> 
>>> On Feb 4, 2015, at 3:46 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>> 
>>>> On 2015-Feb-04, at 15:37, Frédéric Riss <friss at apple.com> wrote:
>>>> 
>>>>>> 
>>>>>> - The word 'context' is overloaded: `MDNode::getContext()` already
>>>>>> exists, and returns an `LLVMContext&`; `DIDescriptor` uses 'context'
>>>>>> to mean "the node that this one is defined inside".  I chose the
>>>>>> word 'parent' instead of 'context' here.  Is this word okay?  If
>>>>>> not, what about 'scope'?  This will be reflected in the assembly
>>>>>> changes to come (I'd like the C++ names to match the assembly names,
>>>>>> although technically it's not necessary).
>>>>>> 
>>>>>> I'd /probably/ go with scope (we already have scope in the MDLocations, so that seems consistent), but fairly on-the-fence.
>>>>>> 
>>>>> 
>>>>> Weirdly, I didn't even notice that :).  In that case I like 'scope'
>>>>> better too.  I'll update to that before commit.
>>>> 
>>>> Seems most natural. Can the futur getScope() return something that doesn’t derive from MDScope?
>>> 
>>> I don't think so.
>> 
>> That was my impression also, and it makes it even more appropriate IMHO.
>> 
> 
> Just a few general remarks to throw into the discussion:
> 
> - Would it make sense to use something like tablegen to generate the repetitive parts? I’m slightly worried about copy&paste bugs, moderately worried about refactoring it in the future.
> 

`tablegen` might make sense here, I'm not sure.  Frankly, I've never used
it before, and it's not clear to me whether it would actually reduce the
amount of code in this case vs. shifting it over to utils/tablegen (given
the macro usage).

I think the way the APIs are written, the only real danger is a mismatch
between `MDNodeKeyImpl<>` and the current fields in the class (I believe
other mismatches would be caught by the compiler).  As long as the unit
tests are updated along with any refactoring that happens I think the
danger even there is fairly low.

I'm open to it if there's definite value, though.  Maybe as a follow-up?

@dblaikie: I've seen you poking around tablegen internals a fair bit; any
thoughts on this one?

> <classllvm_1_1DIScope__inherit__graph.png>
> 
> As for scopes, there are several things that bug me about the current class hierarchy that we could fix now:
> - DIFile should not be a scope (the concept of files is IMO orthogonal to scoping and there is always something more appropriate to put a node into: compile unit, module, namespace)
> - DIBasicType should not be scope
> - It’s questionable whether a DICompositetype should be a DIDerivedType
> - A DISubroutineType should be neither a DIDerivedType nor DIScope
> - Using DIDerivedTypes for CV qualifiers is a bit wasteful but it does map nicely to DWARF
> 

Definitely want to get to those :).  I think most of those are marked
in the code as `TODO`s (except for the CV-qualifier comment).  But I'm
planning to change the schema itself separately from the infrastructure
for it to simplify the triage of any problems that come up (see comments
in PR22264).

> otherwise, thanks for doing this!
> -- adrian





More information about the llvm-commits mailing list