<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 1, 2013 at 2:34 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"><br><br><div class="gmail_quote">
<div class="im">On Tue, Oct 1, 2013 at 2:18 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 1:57 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"><br><br><div class="gmail_quote">
<div>On Tue, Oct 1, 2013 at 12:52 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">Author: mren<br>
Date: Tue Oct 1 14:52:23 2013<br>
New Revision: 191792<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=191792&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=191792&view=rev</a><br>
Log:<br>
Debug Info: remove duplication of DIEs when a DIE is part of the type system<br>
and it is shared across CUs.<br></blockquote><div><br></div></div><div>This may require some discussion/consideration.</div><div><br></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>Hi David,</div><div><br></div><div>Thanks for reviewing the commit. I don't quite get why the parent/child relationship is going to change.</div><div>The patch removes duplication of DIEs so we don't create one DIE for each CU that corresponds to the same MDNode.</div>
<div>We are not making the DIE to have multiple parents. Instead we may refer to the DIE in another CU.</div></div></div></div></blockquote><div><br></div></div><div>Yep, my mistake - this won't affect parent/child relationships, just references.<br>
<br>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>Yes, the DIE will be referenced across-CUs. We have been using ref_addr for some cases, and lldb is fine with it.</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 class="im"><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">
<br>
We add a few maps in DwarfDebug to map MDNodes for the type system to the<br>
corresponding DIEs: MDTypeNodeToDieMap, MDSPNodeToDieMap, and<br>
MDStaticMemberNodeToDieMap. These DIEs can be shared across CUs, that is why we<br>
keep the maps in DwarfDebug instead of CompileUnit.<br></blockquote><div><br></div></div><div>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)<br>
</div></div></div></div></blockquote><div><br></div></div><div>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. </div></div></div></div></blockquote>
<div><br></div></div><div>I tend to agree, to a degree. But what I meant is that we might be able to make support for this more generic, rather than only doing this for specific kinds of entities. If the DIE map moved up from CompileUnit to DwarfDebug we'd just generically add/check for things there, rather than doing this for specific elements.<br>
<br>But maybe this won't work - perhaps that would cause us to find, say, a namespace from one CU and add elements from another CU to that namespace.<br><br>I suppose we'd need a different lookup for when we're adding children or attributes, rather than just adding references. Which is perhaps where you've already arrived (sorry, have to think through this all to catch up to you) - adding only elements that we might want to reference (well, no, that's incomplete - what about a namespace alias? In that case we could, technically, reference a namespace from an existing CU rather than introducing a new copy in the current CU) and then looking them up in the relevant contexts.<br>
</div></div></div></div></blockquote><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><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>I don't quite get why we need two mappings for one MDNode.</div><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><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 class="im">
<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>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><br></div><div>Thanks,</div><div>Manman</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><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> </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>
<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>
We add addDIEEntry to DwarfDebug to be a wrapper around DIE->addValue. It checks<br>
whether we know the correct form, if not, we update the worklist<br>
(DIEEntryWorklist).<br>
<br>
A testing case is added to show that we only create a single DIE for a type<br>
MDNode and we use ref_addr to refer to the type DIE.<br>
<br>
Modified:<br>
llvm/trunk/lib/CodeGen/AsmPrinter/DIE.cpp<br>
llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h<br>
llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp<br>
llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h<br>
llvm/trunk/test/Linker/type-unique-simple-a.ll<br></blockquote></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><div>
Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h?rev=191792&r1=191791&r2=191792&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h?rev=191792&r1=191791&r2=191792&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h (original)<br>
+++ llvm/trunk/lib/CodeGen/AsmPrinter/DIE.h Tue Oct 1 14:52:23 2013<br>
@@ -152,6 +152,9 @@ namespace llvm {<br>
/// Climb up the parent chain to get the compile unit DIE this DIE belongs<br>
/// to.<br>
DIE *getCompileUnit();<br>
+ /// 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><div><div>Why is this not just "getParent() == NULL"? Why do you need a walk for the DIE?</div></div></div></div></div></blockquote>
<div>Yeah, that should work.</div>
<div><br></div><div>Thanks,</div><div>Manman </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> </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">
void setTag(uint16_t Tag) { Abbrev.setTag(Tag); }<br>
void setOffset(unsigned O) { Offset = O; }<br>
void setSize(unsigned S) { Size = S; }<br><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>