[PATCH] IR: Add specialized debug info metadata nodes

[Duncan P. N. Exon Smith]

> On 2015-Feb-04, at 17:17, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2015-Feb-04, at 16:44, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>>> 
>>> 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
>> 
> 
> Here's an updated patch using "scope" instead of "parent" (and updated
> to match r228242, from David's review of r228212).
> 

*ping*

(While the discussion about the final structure of the class hierarchy
and scopes/types/etc. is a good one, it's somewhat independent of the
patch in question.)

This is a fast ping, but I'm just about blocked on this, and with all the
discussion about the future I'm afraid everyone forgot to sign off on
this step forward.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-IR-Add-specialized-debug-info-metadata-nodes.patch
Type: application/octet-stream
Size: 156092 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150206/17be6943/attachment.obj>