[llvm] r191792 - Debug Info: remove duplication of DIEs when a DIE is part of the type system

[Manman Ren]

On Tue, Oct 1, 2013 at 1:57 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> On Tue, Oct 1, 2013 at 12:52 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>> Author: mren
>> Date: Tue Oct  1 14:52:23 2013
>> New Revision: 191792
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=191792&view=rev
>> Log:
>> Debug Info: remove duplication of DIEs when a DIE is part of the type
>> system
>> and it is shared across CUs.
>>
>
> This may require some discussion/consideration.
>
> The first thing that comes to my mind is that this is changing the DIE
> structure from trees to DAGs and, perhaps importantly, is going to make
> parent/child relationships incomplete (ie: A child of B no longer implies
> that B is the parent of A) which is a bit concerning.
>

Hi David,

Thanks for reviewing the commit. I don't quite get why the parent/child
relationship is going to change.
The patch removes duplication of DIEs so we don't create one DIE for each
CU that corresponds to the same MDNode.
We are not making the DIE to have multiple parents. Instead we may refer to
the DIE in another CU.


>
>
>>
>> We add a few maps in DwarfDebug to map MDNodes for the type system to the
>> corresponding DIEs: MDTypeNodeToDieMap, MDSPNodeToDieMap, and
>> MDStaticMemberNodeToDieMap. These DIEs can be shared across CUs, that is
>> why we
>> keep the maps in DwarfDebug instead of CompileUnit.
>>
>
> Why nod simply map MDNode* to DIE the same as we do in the CompileUnit? We
> should be able to do this for all DIEs, no? (essentially just move the map
> up from CompileUnit to DwarfDebug)
>

Yes, we can use a single map instead of 3 maps, I figured that with 3 maps,
each map can be smaller and it is faster to query.

>
> But I'm not sure how effective this would be - like I said, might need
> some thought/discussion to understand your solution & consider alternatives.
>
>
>>
>> Sometimes, when we try to add an attribute to a DIE, the DIE is not yet
>> added
>> to its owner yet, so we don't know whether we should use ref_addr or ref4.
>> We create a worklist that will be processed during finalization to add
>> attributes with the correct form (ref_addr or ref4).
>>
>
> Do you have an example of this as a test case? (I really wish we had an
> easy way to see test coverage - so I could just tell at a glance whether
> the worklist code is being exercised by your test cases... oh well)
>
Yes, we don't always immediately add a newly-created DIE to a parent. So it
is possible to have DIEs without an owner when calling addDIEEntry.
One interesting example is:
getOrCreateTypeDIE --> addToContextOwner --> getOrCreateContextDIE (the
original type DIE can be referenced during the construction of the context,
and when that happens, the type DIE does not have an owner yet)


>
>
>>
>> We add addDIEEntry to DwarfDebug to be a wrapper around DIE->addValue. It
>> checks
>> whether we know the correct form, if not, we update the worklist
>> (DIEEntryWorklist).
>>
>> A testing case is added to show that we only create a single DIE for a
>> type
>> MDNode and we use ref_addr to refer to the type DIE.
>>
>> Modified:
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>     llvm/trunk/test/Linker/type-unique-simple-a.ll
>>
>
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h?rev=191792&r1=191791&r2=191792&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h (original)
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h Tue Oct  1 14:52:23 2013
>> @@ -152,6 +152,9 @@ namespace llvm {
>>      /// Climb up the parent chain to get the compile unit DIE this DIE
>> belongs
>>      /// to.
>>      DIE *getCompileUnit();
>> +    /// Similar to getCompileUnit, returns null when DIE is not added to
>> an
>> +    /// owner yet.
>> +    DIE *checkCompileUnit();
>>
>
> Why is this not just "getParent() == NULL"? Why do you need a walk for the
> DIE?
>
Yeah, that should work.

Thanks,
Manman

>
>
>>      void setTag(uint16_t Tag) { Abbrev.setTag(Tag); }
>>      void setOffset(unsigned O) { Offset = O; }
>>      void setSize(unsigned S) { Size = S; }
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131001/5a3f1d17/attachment.html>