<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div><br></div><div><br>On Oct 2, 2013, at 4:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Oct 2, 2013 at 4:05 PM, 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 10:42 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@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 dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Tue, Oct 1, 2013 at 7:05 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div>I am now a little confused about the mapping issue.</div>
<div>The patch replaces every getDIE of a Type, a SP, and a Static Member with DwarfDebug.getType|SP|StaticMemberDie.</div>
<div>
<br></div><div>getDIE was used before creating a DIE, and it was also used before generating a reference to the DIE via ref4 or ref_addr.</div><div><br></div><div>For the usage before creating a DIE, if we still use the CU mapping, we will create multiple DIEs for the same MDNode across CUs, if we replace it with the DwarfDebug mapping, we will not create multiple DIEs.</div>
<div><br></div><div>For the usage before generating a reference, it does not make sense to use CU mapping if we only have a single DIE for the MDNode (i.e using DwarfDebug mapping before creating a DIE); it does not make sense to use DwarfDebug mapping if we already</div>
<div>have multiple DIEs for the MDNode.</div><div><br></div><div>So it sounds to me, the usage of getDIE before creating a DIE determines whether we have multiple DIEs for a given MDNode and other usages of getDIE should either use CU mapping or DwarfDebug mapping, but not both for a given MDNode.</div>
</div></blockquote><div><br></div></div><div>OK, I think I'm understanding you here. Just throwing around thoughts, nothing terribly deeply analyzed yet, bu it might be easier to understand if we simply had CompileUnit::getDIE have a list of tags for which it would delegate to the cross-CU mapping. Then it'd be simply one mapping in DwarfDebug used from one place (getDIE) and would be hidden from all callers, keeping the rest of the code fairly consistent/simple... just a thought, at least.<br>
</div></div></div></div></blockquote><div><br></div></div><div>Sounds reasonable, I will work on that.</div><div class="im"><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>But specifically you're just interested in sharing types between CUs, yes? The sharing of Subprograms is just really for sharing them when they're members of a type, yes?</div></div></div></div></blockquote></div>
<div>
Yes. </div><div class="im"><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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">
<div> (I suppose subprograms could appear in multiple CUs if they were inline in a header and used in multiple CUs)<br>
<br>Would it perhaps be easier to remove this patch and just use type units that I'm working on implementing at the moment?<br></div></div></div></div></blockquote><div><br></div><div><br></div></div><div><div>This patch does two things:</div>
<div>1> move the map from CompileUnit to DwarfDebug</div><div>2> correctly use ref_addr if necessary (by using a worklist)</div></div><div><br></div><div>There are some functionality overlapping between this patch and the type units, but there are certainly differences as well.<br>
</div></div></div></div></blockquote><div><br></div><div>I'm not so interested in a comparison of their implementation as I am in a comparison of their observable result to debug info.</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>
</div><div>Both need to have a map in DwarfDebug that maps type MDNodes to DIE(s), in your proposed patch,<br></div><div>DenseMap<const MDNode *, std::pair<uint64_t, SmallVectorImpl<DIE*>* > > TypeUnits</div>
<div> --> I am not quite sure why we need the pair, can you add some comments in your proposed patch? :)</div></div></div></div></blockquote><div><br></div><div>It was mostly just out as a straw man. The pair is a semi-descriminated union, the vector is your 'work list', I believe. While we're generating the type unit for some type T, the pair will have a non-null vector and an irrelevant uint64 (because we haven't finished generating the type, so we can't compute the hash for it). When a non-null vector is seen, DIEs that should be skeletons for the type unit are added to the vector. When we finish a type unit, we attach the signature to all those DIEs. From then on, whenever we're looking to build a skeleton DIE for a type, we have the signature in the uint64_t (because the vector pointer is null, we know the signature is valid) and we just attach it immediately.</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>In my patch, we have a DenseMap<const MDNode *, DIE*> MDTypeNodeToDieMap</div>
<div><br></div><div>
With type units, the across-CU references will be replaced with references to type units.</div><div>With this patch, we use ref_addr.</div><div><br></div><div>For LTO's purpose, we don't need to calculate the hash since all we need is to make sure we only generate a single DIE for a given type MDNode.</div>
<div>Calculating the hash costs time :)</div></div></div></div></blockquote><div><br></div><div>Enough to matter? I wonder.<br></div></div></div></div></div></blockquote>I wonder about that too. Can we do any measurement now?<br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>Of course type units will duplicate more things - they'll have the indirection of the type skeleton (a structure/class_type with a signature attribute), and duplicate member functions for implicit members (nested types, member template instantiations, and implicit special member functions) and function definitions. But I don't know just how significant that win will be/whether it'll be worth doing when we get most of the win from type units - maybe the extra won't be enough to justify the added complexity. (indeed this commit didn't come with any numbers about how much it helps)</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 would like to revisit this (maybe revert this) later on when type units are working.<br>
</div></div></div></div></blockquote><div><br></div><div>I'd rather revert this before type units (& then revisit this afterwards) as it may complicate the type unit work.</div></div></div></div></div></blockquote>What kind of complication do you see ?<div><div><br></div><div>Manman<br><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div></div></div></div></div></blockquote><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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 class="im"><div><br></div><div><blockquote class="gmail_quote" style="font-family:arial,sans-serif;font-size:13px;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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
<div>+ /// Similar to getCompileUnit, returns null when DIE is not added to an<br>+ /// owner yet.<br>+ DIE *checkCompileUnit();<br></div></blockquote><div><br></div></div><div>Why is this not just "getParent() == NULL"? Why do you need a walk for the DIE?</div>
</div></div></div></blockquote><div style="font-family:arial,sans-serif;font-size:13px">Yeah, that should work.</div></div></div><div style="font-family:arial,sans-serif;font-size:13px"><-- It is possible that a DIE is added to a parent but the parent is not yet added to an owner yet, so we still have to walk along the parent chain.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div>Thanks,</div><div>Manman</div><div><div class="h5"><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>While I have discussed with Eric the idea that DIEs could be referenced cross-CU, I'm a bit hesitant to put this sort of thing in without further thought/consideration, and it seems like type units are a more general purpose solution to this and other issues anyway. (though they don't immediately address the inline non-member function example I mentioned above, those would still be duplicated across CUs - so if that's really important we might have to do something like this anyway)</div>
<div><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"><div dir="ltr">
<div><br></div><div>Thanks,</div><div>Manman</div></div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 1, 2013 at 5:58 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Tue, Oct 1, 2013 at 4:10 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@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 dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>The idea is that we're going to have references across-CUs? Do you have any idea if debuggers tolerate/handle this kind of referencing?</div></div></div></div></blockquote></div><div>Yes, the DIE will be referenced across-CUs. We have been using ref_addr for some cases, </div>
</div></div></div></blockquote><div><br></div></div><div>In which cases do we already do this?</div></div></div></div></blockquote></div><div>In DwarfDebug::updateSubprogramScopeDIE, when we add abstract_origin of a subprogram.</div>
<div>I agree that we are now using ref_addr broadly for LTO builds.</div><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
<div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div>and lldb is fine with it.</div></div></div></div></blockquote><div><br></div></div><div>lldb isn't the only DWARF consumer.</div></div></div></div></blockquote></div>
<div>
Other debuggers should work with ref_addr.</div><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><br></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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I tried to move the mapping for MDNodes related to the type system, from CompileUnit to DwarfDebug, and I didn't include namespace in this commit.</div>
<div>Theoretically, for everything that can be shared across CUs, we should add it to the DwarfDebug map to remove duplication.</div></div></div></div></blockquote><div><br></div></div><div>Then it would be nice to do this as a single, generic solution over all things that can be referenced rather than piecemeal with special cases for each kind of thing, I think.</div>
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<br>So a strawman alternative proposal would be: add all the MDNode/DIE mappings to both the local CompileUnit and 'global' DwarfDebug maps and have different lookup functions depending on the context - using the DwarfDebug mapping when we want to reference anything for any reason, and the CompileUnit mapping when we want to add children (eg: adding new things to a namespace, etc) or attributes.</div>
</div></div></div></blockquote></div><div>I don't quite get why we need two mappings for one MDNode.</div></div></div></div></blockquote><div><br></div></div><div>Because, say we have a namespace node - we need a CU-specific mapping so that when we need to add entities to the namespace (another type, global variable, etc) we can lookup the CU-local instance of the namespace. But whewhy we need two mappings for one MDNode.</div></div></div></div></blockquote><div><br></div></div><div>Because, say we have a namespace node - we need a CU-specific mapping so that when we need to add entities to the namespace (another type, global variable, etc) we can lookup the CU-local instance of the namespace. But when we only need to refer to the namespace (eg: for a namespace alias) we can lookup a cross-CU mapping.<br>
</div></div></div></div></blockquote></div><div>If we already have multiple DIEs for the same MDNode (that is why we keep a CU-specific mapping, right?), when referring to the namespace DIE, can we just use CU-specific mapping?</div>
<div>I am not familiar with how a namespace alias works, so don't quite know why we need a cross-CU mapping when referring to the namespace.</div><div><div><br></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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>This is exactly what you've already implemented - the mappings you added to DwarfDebug are the second mapping - the first mapping already exists within the CompileUnit and is still used for the cases where we want to add members (at least I hope we are, because we need to do that in some cases).</div>
</div></div></div></blockquote></div><div>Oops, I moved the mapping from CompileUnit to DwarfDebug. Are you referring to "the namespace alias" by "some cases"? </div><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>I tried to categorize MDNodes to two groups, one group belonging to CompileUnit (Variables etc that will be referenced in a single CU), the other group belonging to DwarfDebug (type system that can be shared across CUs).</div>
</div></div></div></blockquote><div><br></div></div><div>I don't believe that will be sufficient, but I could be wrong. Namespaces seem like a pretty solid example where a thing would need to be mapped in both a CU-specific and cross-CU way.<br>
<br>For types, currently there are cases where we insert member DIEs into type DIEs lazily (related to the vtable debug info optimization, wherein we emit only a declaration of a type when that type is dynamic (has a vtable) and the vtable isn't emitted in this translation unit).</div>
</div></div></div></blockquote></div><div>Do we have an existing testing case for this? I can try to extend it to multiple CUs to check what will happen.</div><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> It might not be wrong to have us insert those member DIEs in an existing instance of the type in another compile_unit - but I'm hesitant to assume that's good and right.</div>
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>But I'm not sure how effective this would be - like I said, might need some thought/discussion to understand your solution & consider alternatives.</div><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>
Sometimes, when we try to add an attribute to a DIE, the DIE is not yet added<br>
to its owner yet, so we don't know whether we should use ref_addr or ref4.<br>
We create a worklist that will be processed during finalization to add<br>
attributes with the correct form (ref_addr or ref4).<br></blockquote><div><br></div></div><div>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)</div>
</div></div></div></blockquote></div><div>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.</div><div>One interesting example is:</div>
<div>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)</div>
</div></div></div></blockquote><div><br></div></div><div>But you know that the reference to that type DIE will be from within the same CU as the type itself - since it's a child. So you know the reference will be a ref4, no?</div>
</div></div></div></blockquote></div><div>For this case, yes, it is going to be ref4. But I am not sure whether we can say the same for all cases when a DIE is not added to an owner.</div><div>It also means we need to pass in an extra flag telling addDIEEntry that we are sure it is a ref4 and it is not an easy task to track the flag across the chain of</div>
<div>function calls: <span style="color:rgb(80,0,80)">getOrCreateTypeDIE --> addToContextOwner --> getOrCreateContextDIE --> ... --> addDIEEntry</span>.</div></div></div></div></blockquote><div><br></div></div>
<div>
Why would you need an extra flag? If we know that any time we come back around to the same entity that isn't already inserted (ie: the condition you already know because you use it to decide to add things to a work list) it must be a local reference, so you answer the question immediately rather than using a work list.<br>
</div></div></div></div></blockquote></div><div>How can we tell that we are coming back around to the same entity? Maybe I misunderstood something here.</div><div>When we see a DIE without an owner and add things to a work list, I don't think there is a guarantee that the DIE and the Entry belong to the same CU.</div>
<div><br></div><div>Thanks,</div><div>Manman</div><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>I could be wrong, but I want to have a good reason to do it some other way not just "maybe sometimes this might not happen".<span><font color="#888888"><br><br>- David</font></span></div><div><div>
<div> </div></div></div></div></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br>
<br>
<br>
<br>
<br>
<br>
<br>
div>
</div></blockquote><blockquote type="cite"><div><span>_______________________________________________</span><br><span>llvm-commits mailing list</span><br><span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a></span><br><span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></span><br></div></blockquote></div></div></div></body></html>