<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Jun 21, 2013, at 4:01 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Fri, Jun 21, 2013 at 3:58 PM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Jun 21, 2013, at 3:25 PM, David Blaikie wrote:<br><br><blockquote type="cite">On Fri, Jun 21, 2013 at 3:16 PM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Jun 21, 2013, at 1:12 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><br>On Fri, Jun 21, 2013 at 12:33 PM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:<br><br><br>On Jun 21, 2013, at 11:56 AM, Eric Christopher wrote:<br><br>On Fri, Jun 21, 2013 at 11:50 AM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:<br><br><br>On Jun 21, 2013, at 11:35 AM, Eric Christopher wrote:<br><br>On Thu, Jun 20, 2013 at 10:52 PM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:<br><br><br>A summary of options for issue #3:<br>3> To actually access the MDNode referenced via the hash value, we need to<br>perform a lookup from the hash value to find the corresponding MDNode.<br>The questions are where to store this map and how to keep it up-to-date when<br>a MDNode is replaced.<br>---------------------<br>Option a) a map in DwarfDebug, AsmWriter and DIBuilder, modify access<br>functions in DI classes to pass in the map.<br>May need to modify printing functions in AsmWriter.<br><br><br>You may not need the map in DIBuilder if you piggy back on the<br>existing map in CGDebugInfo.cpp. The AsmWriter stuff is unfortunate<br>for just debug dumps of the IR. A way around this might be nice, but<br>I've not looked into it.<br><br><br>Calling dump() on a MDNode inside gdb will not give us the derived type<br>information, since we don't have a handle to the map.<br><br><br>The only case where dump currently navigates a type link that I know<br>is printing derived types, where we print "[from foo]". By just<br>printing the string identifier for the type (without having to<br>navigate the link/use the map) should be about as useful here.<br><br><br><br>---------------------<br>Option b) I am going to expand David's description with more details,<br>correct me if I am wrong.<br><br><br>I'll let David continue this part of the thread with you.<br><br><br>I personally prefer option b over a since it is much cleaner.<br><br><br>Option b is more general and should probably work. I suggested a<br>originally because I know it would work and the impact is limited to<br>debug info. Also it doesn't involve touching the generic metadata<br>interface or code and could be done quickly and incrementally. b may<br>also require further changes to the debug info infrastructure to deal<br>with lookup, etc so may end up being as much churn as a. b is more<br>useful if we think other users of metadata may want/need similar<br>functionality in the future.<br><br>I'm fine with b as a choice, but it's going to involve more planning<br>and code review and involve people outside of you, me and Dave :)<br><br><br>Got it. David, are you okay with the details I added about option b?<br>Let's start a new thread with title "Proposal: extend Metadata (MDNode) to<br>support MDHash" or something like that.<br><br><br>I'd like to hear a rationale for wanting b first. I don't know (and<br>can think of) any other users of this functionality right now.<br><br><br>I will try my best :]<br>MDNode has a list of operands, one MDNode's full content can be thought of<br>as a graph, when there is a reference to another MDNode, we have<br>an edge. The MDHash can be used to give a unique id (or string) to the full<br>content of the graph.<br><br>The hash value applies to any kind of MDNode with complicated MDNode<br>references.<br><br>If you think this is not general enough to be in MDNode,<br><br><br>Eric's concern/question is whether it's a feature that would be useful<br>to any other code, I believe. (I'm less concerned about this so long<br>as the feature is appropriately general, even if it doesn't have any<br>other consumers at the moment - I don't think it'd drastically<br>complicate the Metadata handling code (which I think is currently<br>fairly simple, but I admit to not knowing it in any great detail))<br><br><br>Yes, I tried to explain why MDHash can help with MDNodes that form a<br>complicated graph.<br><br><br><br>my earlier proposal of extending MDNode to DINode is going the other<br>direction.<br><br><br>As discussed, this isn't far enough in the other direction. If we're<br>going to touch core LLVM Metadata handling code at all, it needs to be<br>a complete/cohesive feature, not just tendrils of a debug info<br>feature. Hence (a) or (b) not (c).<br><br>Basically, MDNode will have an opcode like the opcode for Instruction, to<br>support TBAA, DI, and other types.<br>Inside streamers (bc ll reader), we can create the specialized class<br>according to the opcode.<br>That requires a lot of changes too :(<br><br><br>We can add a weight to MDHash (other names are fine with me), to support<br>multiple MDNodes with the same hash, but different weight.<br><br><br>Wait, what? This sounds like a bad idea.<br><br>Why?<br>The weight is needed to differentiate a forward declaration vs. a<br>definition, they both name the same class, but they are different.<br><br><br>This would probably tip me over into Eric's perspective that it would<br>be too esoteric a feature to add to a generic/core piece of<br>infrastructure.<br><br><br>If we allow the map to map one hash value (or string) to multiple MDNodes,<br>without adding the weight, it should still work.<br>But is that general enough?<br></blockquote><br>Then there would be no transparent access through the nodes - so I<br>don't know what the access API would look like anymore & it would seem<br>rather awkward as a general API.<br></blockquote><br>The access function will have two modes:<br>1> it does not matter which one is returned<br>(This works for verification purposes and printing purposes.)<br>2> return all the MDNodes having the hash, DwarfDebug will figure out which one has priority<br><br>It is not clean. I actually prefer adding a weight, it kind of makes sense when we are dealing with multiple MDNodes with the same hash.<br>With weight, we can always pick one with higher weight (or priority).<br><br>If we veto option b, we will only have option a, which has its own issues:<br>1> it is not really local to debug info, we have to modify AsmWriter at least.<br></blockquote><br>I'm pretty sure I've kept saying we don't need to do this. Printing<br>asm can just print the identifier for the type node - it doesn't need<br>to navigate across that link.<br></div></blockquote><div><br></div>Okay, if we are fine with no access to the link in a gdb | lldb session.</div><div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">2> we have to keep (and construct) one map at a few places.<br>3> we have to modify the following access function in DI classes<br>getContext, getTypeDerivedFrom, getClassType, getContainingType, getType<br>to take an extra map<br>and of course the call sites of all these functions.<br><br>Compare against putting the map in LLVMContext:<br>1> quoting from David<br>"General principle of keeping things in the least scope necessary.<br>Reducing scope helps code<br>maintenance by making the influence of certain data is only accessed<br>within a certain scope."<br><br>Another issue is on how to submit the patches:<br>I prefer to submit small patches and have a transition period where we use -gtype-uniquing to test the work in progress.<br></blockquote><br>Incremental work doesn't require a flag. You can simply migrate one<br>field at a time. Is there a reason that wouldn't work?<br></div></blockquote><div><br></div><div>The reason I want a flag is to avoid the need to update the existing testing cases while this is a work in progress.</div><div>I believe migrating one field means updating the existing testing cases?</div></div><div><br></div><div>Thanks,</div><div>Manman</div><div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">The advantages are<br>1> People can see our progress.<br>2> We can incrementally add more testing cases while testing the feature under the flag.<br>3> If the patch causes problem, we don't have to revert the big patch and submit another big patch that has the fix.<br><br>Thanks,<br>Manman<br><br><blockquote type="cite"><br>The more you point out the reasonable complications in using such a<br>generic API for your task, the less it seems like it would be<br>sufficient/a good fit - the more I'd be inclined to go with Eric's<br>choice of just keeping the mapping locally (in DwarfDebug, etc) & not<br>impacting metadata APIs at all.</blockquote></blockquote></div></blockquote></div><br></body></html>