<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 16, 2014 at 2:18 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2014-Dec-16, at 12:49, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> Should "OverridingFunctions" be "OverriddenFunctions"?<br>
<br>
</span>Nope, these are the overriding functions.  The functions they've<br>
overridden have been deleted and no longer exist.<br>
<span class=""><br>
> (it looks like everything in that set gets dropped from the subprogram lists - so if those are the functions chosen to be kept by the linker, I would think that would end up with them all being dropped from the debug info - the exact opposite of what's desired - but I assume I'm misunderstanding something)<br>
<br>
</span>The timing is important.<br>
<br>
When this function is called, `OldF->replaceAllUsesWith(NewF)` has been<br>
called, so all the old subprograms will be pointing at the NewF.<br></blockquote><div><br>Ah<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">However, the named metadata hasn't been linked yet, so the new compile<br>
units aren't yet in `DstM`.<br></blockquote><div><br>OK. & I take it we always favor/keep the function from SrcM? (what about cases where we can't - weak in source, strong in destination?)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">><br>
> (stylistically I'd remove a bunch of the conditionals in that code as not too important: if the function set is empty, everything else should be fairly trivial, I don't necessarily know that the early return is particularly valuable.<br>
<br>
</span>This function gets called each time a module gets linked in.<br>
Unnecessarily traversing all the compile units and their subprograms is<br>
wasteful.<br>
<br>
Moreover, this code is quadratic in the worst case, since it gets called</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
once for every module that's linked in (and it traverses all the compile<br>
units added so far).</blockquote><div><br>Should we wait and do it just once at the end of linking all the modules in, then?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  In practice, most `SrcM` won't have functions that<br>
have overridden something in `DstM`. </blockquote><div><br>Pretty much any C++ module it's going to be true for, I'd imagine - something as simple as std::string::empty is probably in almost any TU, eventually, for example (or take some similarly common function).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Returning early here is important.<br></blockquote><div><br>I realize the counterpoint to premature optimization is don't write silly inefficient code, but I'd be surprised if any of this showed up in profiles, though I could be wrong - don't have nearly the intuition/experience in LTO performance profiles that you do.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Two calls to getNamedMetadata (& two checks of the same non-null result)<br>
<br>
</span>Actually, these are two different results.  One is for the `SrcM`, and<br>
the other is for the `DstM`.  If there is no compile unit in the `SrcM`,<br>
then I'm leaving the subprograms as they are the `DstM`, since there's no<br>
collision.<br></blockquote><div><br>Right right.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> . CUs in the CU list should never be null, the subprogram array should never be null (I think that's the case?).<br>
<br>
</span>This might be true... I can change these to assertions.  However<br>
depending on how I resolve Rafael's concern about symmetry, there could<br>
be nulls in the array of subprograms.  Do you think that's a problem?<br></blockquote><div><br>Yeah, I'd rather avoid introducing that possibility - I'm pretty sure I've removed a bunch of conditionals that might've allowed that at some point & I'd wager we'll assert pretty pervasively if there are null elements in the subprogram list.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(Still, I'd rather add the code with conditions and switch it to<br>
assertions in a separate commit, so that if the assertions fire we know<br>
it's unrelated to the other semantic changes.)<br></blockquote><div><br>So long as it follows shortly after I don't much mind which way you do this - I just would really like to avoid being too loose with these things, such code has hid a miriad of mistakes on a regular basis.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> & I might even not bother with the old size != new size condition either - just to keep the code simple/easy to read)<br>
<br>
</span>Calling `replaceSubprograms()` creates a lot of function traffic, and<br>
`MDNode::get()` isn't guaranteed to return the same `MDNode` (since the<br>
one there isn't guaranteed to be in uniquing mode).  I'd rather save<br>
memory and time, since LTO is already bad enough.<br></blockquote><div><br>Again, your call - you know the perf profiles of this stuff & I've no clue. I'd just be surprised if this showed up.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> - David<br>
><br>
> On Tue, Dec 16, 2014 at 12:14 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2014 Dec 16, at 10:27, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On Tue, Dec 16, 2014 at 10:06 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> ><br>
> > > On 2014-Dec-16, at 10:00, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > ><br>
> > ><br>
> > ><br>
> > > On Tue, Dec 16, 2014 at 9:48 AM, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
> > > > It's possible to put DWARF subprogram definitions for linkonce_odr functions<br>
> > > > into comdat groups so they are dropped along with the linkonce_odr function,<br>
> > > > but Clang (& I don't think GCC by default) does that (there's some overhead<br>
> > > > to that representational choice, etc). But yes, it can be done. I think<br>
> > > > today, for both GCC and Clang, you'd end up with two subprogram definitions<br>
> > > > (one in each DWARF compile unit) each pointing to the same high_pc/low_pc to<br>
> > > > describe the function.<br>
> > ><br>
> > > I thought this was part of a special case for dwarf in linkers (and<br>
> > > the missing feature why lld produced binaries are much larger in -g<br>
> > > builds).<br>
> > ><br>
> > > To the best of my knowledge, at least the usual Linux linkers (gold and binutils-ld) don't have any special cases for DWARF (well, gold has some magic to generate a GDB debug info index, maybe), it's just sections and relocations like any other data in an object file. /maybe/ dsymutil on MacOS does, but I've not heard about it. (& possibly dwz - a debug info-aware compression tool could do that trick, it's designed to eliminate redundancy in debug info)<br>
> > ><br>
> > > A basic test case shows roughly what I described. (attached dwarfdump, if you're curious - you can see what the source would've looked like from the DWARF there (inline function in a header, two translation units that instantiate the inline function, etc))<br>
> > ><br>
> > ><br>
> > > > (but yeah, vaguely sounds like what you're suggesting would make sense - I<br>
> > > > haven't looked at the initial proposed patch yet, though)<br>
> > ><br>
> > > Cheers,<br>
> > > Rafael<br>
> > > <dump.txt><br>
> ><br>
> > Even if other linkers don't do it, is it reasonable to delete subprograms from<br>
> > compile units if the canonical one is from another compile unit (and has a<br>
> > subprogram there)?<br>
> ><br>
> > I think so - more importantly, I assume that's what we'll end up doing anyway. (check the actual DWARF output - since we build a map of subprograms, I don't think we end up emitting the subprogram to both CUs anyway - we just ignore all but the first one we find - that's my bet anyway)<br>
> ><br>
> ><br>
> > If so, I can just do that.<br>
><br>
> How does this look?<br>
><br>
<br>
</div></div></blockquote></div></div></div>