<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 3, 2013 at 11:26 AM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="im">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>><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><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></div></div></blockquote><div><br></div><div>Agreed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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></div></div></blockquote><div><br></div><div>I don't quite understand this. The list of changes looks incomplete (the insert*DIE functions in DwarfDebug and the maps they use, etc, are not shown here) - are you describing this as "changes" as distinct from "additions"? Even adding completely new code comes at a maintenance burden that should be weighed against the value added.<br>
<br>& this doesn't look like how I would expect the code to look after my suggestion (for one thing, I think I was suggesting having one map in DwarfDebug, rather than one per kind of thing - and simply having getDIE (and insertDIE, for that matter) delegate based on a list of known tag types that should be cross-CU versus CU-local)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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<br></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></div></div></blockquote><div><br></div><div>Yep, it might come out this cleanly, though I'm a little cautious about putting all these things in together then trying to clean it up/evaluate the changes appropriately.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div></blockquote><div><br></div><div>Certainly - as I said, I don't doubt your change has some value over type units. There's a question as to how much value and how best to fit it in, if we do, to the existing code.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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<br></div><div>without this patch. </div></div></div></div>
</blockquote><div><br></div><div>Yes, I agree that it has value.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>I do have some numbers collected before, I will dig that out.</div></div></div></div></blockquote><div><br></div><div>Thanks, that'd be very helpful.<br><br>(but I'm also particularly interested in how much value this has over type units - which is one reason I'd like to do this after type units, either that or we'll need to add flags or somesuch to disable this optimization so we can compare type units with and without this change)<br>
<br>The other problem is that removing these patches will be somewhat more difficult after I've spent the next couple of weeks cleaning code up and adding in type units. I think there's enough design discussion to have with regards to how best to implement your feature that I'd rather have that design discussion on top of type units rather than the other way around - though it's possible we can come to a reasonable/good design in either order.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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 class="im"><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><font color="#888888"><br>
-eric<br>
</font></span></blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>