<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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: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 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:0 0 0 .8ex;border-left:1px #ccc 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>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><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> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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>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><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>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 class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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>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><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 class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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></blockquote><div><br></div></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>Yeah, that should work.</div>
<div><br></div><div>Thanks,</div><div>Manman </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 class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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><br></div></div>