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

[Manman Ren]

On Wed, Oct 2, 2013 at 5:55 PM, Eric Christopher <echristo at gmail.com> wrote:

> >
> >>
> >> I would like to revisit this (maybe revert this) later on when type
> units
> >> are working.
> >
> >
> > I'd rather revert this before type units (& then revisit this
> afterwards) as
> > it may complicate the type unit work. I think the extra benefit (using
> > direct ref_addr, rather than signatures) of this approach over type units
> > may be implementable via an incremental improvement to type units.
> >
>
> FWIW I've been following along with the thread and agree here. David
> is actively working on type units and we should have something here
> shortly. Let's let that work proceed and revert this for now.
>

I think we do agree that this approach has some extra benefit (using direct
ref_addr, rather than signatures) over type units.
The type-unit work can achieve what this approach does in removing DIE
duplication, but using signatures rather than ref_addr.
It may be possible to get the extra benefit via an incremental improvement
to type units, but we are not certain about that.

I also want to emphasize this patch after wrapping up CU::getDIE as David
sugggested is not big, the list of changes can be found at the end of this
email.

I don't quite get the complication caused by this approach to type units.
When we start constructing a DIE for a type MDNode, we either use the type
unit or we use
the standard approach  of creating a DIE, my changes touch the standard
approach a little by moving the map from CU to DwarfDebug.
When referring to the type, we either use ref_signature or ref_addr or ref4.

Even after type units start working, people can still choose to use either
type units or ref_addr|ref4 for a period of time before their tool chains
are updated to support type units.
As for the tool chains, extra time may be required to set up a map from the
type signature to the actual address in the Dwarf.

The performance improvement of this patch is kind of straight-forward, if
we have X CUs refer to the same type MDNode, X copies of type DIEs will be
created
without this patch. I do have some numbers collected before, I will dig
that out.

Thanks,
Manman

 15 -  insertDIE(Ty, TyDIE);
 16 +  DD->insertTypeDIE(Ty, TyDIE);

 31 -  insertDIE(SP, SPDie);
 32 +  DD->insertSPDIE(SP, SPDie);

 40 -  insertDIE(DT, StaticMemberDIE);
 41 +  DD->insertStaticMemberDIE(DT, StaticMemberDIE);

 52 +  // Process the worklist to add attributes with the correct form
(ref_addr or
 53 +  // ref4).
 54 +  for (unsigned I = 0, E = DIEEntryWorklist.size(); I < E; I++) {
 55 +    addDIEEntry(DIEEntryWorklist[I].Die, DIEEntryWorklist[I].Attribute,
 56 +                dwarf::DW_FORM_ref4, DIEEntryWorklist[I].Entry);
 57 +    assert(E == DIEEntryWorklist.size() &&
 58 +           "We should not add to the worklist during finalization.");
 59 +  }
 60 +

 68 +
 69 +/// When we don't know whether the correct form is ref4 or ref_addr,
we create
 70 +/// a worklist item and insert it to DIEEntryWorklist.
 71 +void DwarfDebug::addDIEEntry(DIE *Die, uint16_t Attribute, uint16_t
Form,
 72 +                             DIEEntry *Entry) {
 73 +  /// Early exit when we only have a single CU.
 74 +  if (GlobalCUIndexCount == 1 || Form != dwarf::DW_FORM_ref4) {
 75 +    Die->addValue(Attribute, Form, Entry);
 76 +    return;
 77 +  }
 78 +  DIE *DieCU = Die->checkCompileUnit();
 79 +  DIE *EntryCU = Entry->getEntry()->checkCompileUnit();
 80 +  if (!DieCU || !EntryCU) {
 81 +    // Die or Entry is not added to an owner yet.
 82 +    insertDIEEntryWorklist(Die, Attribute, Entry);
 83 +    return;
 84 +  }
 85 +  Die->addValue(Attribute,
 86 +         EntryCU == DieCU ? dwarf::DW_FORM_ref4 :
dwarf::DW_FORM_ref_addr,
 87 +         Entry);
 88 +}



>
> (and no, computing the hash isn't enough to matter time wise on anything)
>
> -eric
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131003/3bf0d64a/attachment.html>