[PATCH] IR: Add specialized debug info metadata nodes
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Feb 9 16:59:10 PST 2015
> On 2015-Feb-09, at 15:20, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> On Feb 6, 2015, at 3:37 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>
>>
>>> 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.
>>
>>
>> <0001-IR-Add-specialized-debug-info-metadata-nodes.patch>
>
> This is fine as far as I am concerned.
>
Thanks for the review!
r228640
More information about the llvm-commits
mailing list