<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Oct 2, 2013 at 5:55 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">><br>
>><br>
>> I would like to revisit this (maybe revert this) later on when type units<br>
>> are working.<br>
><br>
><br>
> I'd rather revert this before type units (& then revisit this afterwards) as<br>
> it may complicate the type unit work. I think the extra benefit (using<br>
> direct ref_addr, rather than signatures) of this approach over type units<br>
> may be implementable via an incremental improvement to type units.<br>
><br>
<br>
</div>FWIW I've been following along with the thread and agree here. David<br>
is actively working on type units and we should have something here<br>
shortly. Let's let that work proceed and revert this for now.<br></blockquote><div><br></div><div>I think we do agree that this approach has some<span style="font-family:arial,sans-serif;font-size:13px"> extra benefit (using direct ref_addr, rather than signatures) over type units.</span></div>
<div><font face="arial, sans-serif">The type-unit work can achieve what this approach does in removing DIE duplication, but using signatures rather than ref_addr.</font></div><div><font face="arial, sans-serif">It may be possible to get the extra benefit via an incremental improvement to type units, but we are not certain about that.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">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.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div>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</div>
<div>the standard approach of creating a DIE, my changes touch the standard approach a little by moving the map from CU to DwarfDebug.</div><div>When referring to the type, we either use ref_signature or ref_addr or ref4.</div>
<div><br></div><div>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.</div><div>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.</div>
<div><br></div><div>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</div><div>without this patch. I do have some numbers collected before, I will dig that out.</div>
<div><br></div><div><font face="arial, sans-serif">Thanks,</font></div><div><font face="arial, sans-serif">Manman</font></div><div><br></div><div> 15 - insertDIE(Ty, TyDIE);</div><div> 16 + DD->insertTypeDIE(Ty, TyDIE);</div>
<div><br></div><div> 31 - insertDIE(SP, SPDie);</div><div> 32 + DD->insertSPDIE(SP, SPDie);</div><div><br></div><div> 40 - insertDIE(DT, StaticMemberDIE);</div><div> 41 + DD->insertStaticMemberDIE(DT, StaticMemberDIE);</div>
<div><br></div><div> 52 + // Process the worklist to add attributes with the correct form (ref_addr or</div><div> 53 + // ref4).</div><div> 54 + for (unsigned I = 0, E = DIEEntryWorklist.size(); I < E; I++) {</div><div>
55 + addDIEEntry(DIEEntryWorklist[I].Die, DIEEntryWorklist[I].Attribute,</div><div> 56 + dwarf::DW_FORM_ref4, DIEEntryWorklist[I].Entry);</div><div> 57 + assert(E == DIEEntryWorklist.size() &&</div>
<div> 58 + "We should not add to the worklist during finalization.");</div><div> 59 + }</div><div> 60 +</div><div><br></div><div> 68 +</div><div> 69 +/// When we don't know whether the correct form is ref4 or ref_addr, we create</div>
<div> 70 +/// a worklist item and insert it to DIEEntryWorklist.</div><div> 71 +void DwarfDebug::addDIEEntry(DIE *Die, uint16_t Attribute, uint16_t Form,</div><div> 72 + DIEEntry *Entry) {</div>
<div> 73 + /// Early exit when we only have a single CU.</div><div> 74 + if (GlobalCUIndexCount == 1 || Form != dwarf::DW_FORM_ref4) {</div><div> 75 + Die->addValue(Attribute, Form, Entry);</div><div> 76 + return;</div>
<div> 77 + }</div><div> 78 + DIE *DieCU = Die->checkCompileUnit();</div><div> 79 + DIE *EntryCU = Entry->getEntry()->checkCompileUnit();</div><div> 80 + if (!DieCU || !EntryCU) {</div><div> 81 + // Die or Entry is not added to an owner yet.</div>
<div> 82 + insertDIEEntryWorklist(Die, Attribute, Entry);</div><div> 83 + return;</div><div> 84 + }</div><div> 85 + Die->addValue(Attribute,</div><div> 86 + EntryCU == DieCU ? dwarf::DW_FORM_ref4 : dwarf::DW_FORM_ref_addr,</div>
<div> 87 + Entry);</div><div> 88 +}</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
(and no, computing the hash isn't enough to matter time wise on anything)<br>
<span class=""><font color="#888888"><br>
-eric<br>
</font></span></blockquote></div><br></div></div>