<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-23 14:03 GMT-07:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>> On 2014-Jul-23, at 10:57, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:<br>
><br>
> Hi,<br>
><br>
> I've attached a patch that forces the per-function profile instrumentation variables to use the private linkage unless they need to use the linkage that keeps only one copy.<br>
><br>
> The private linkage forces the names for those variables to be stripped, reducing the resulting object file size.<br>
><br>
> Alex<br>
</div></div>> <PGOprivateLinkage.patch><br>
<br>
Thanks Alex, this sounds promising.<br>
<br>
1. Can you post numbers for the file size reduction?<br></blockquote><div><br></div><div>Using the original patch I've observed the following reductions:<br><br></div><div>cmake: -6%<br></div><div>FileCheck: -11%<br>
</div><div>llvm-profdata: -13%<br></div><div>llvm-tblgen: -14%<br></div><div>gzip.c: -12.5%<br></div><div>luac: -28%<br></div><div>polarssl/ssl_server: -20%<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
2. I think it makes sense to change any `internal` symbols to<br>
`private`.<br>
<br>
3. I suspect you could change *all* "name" variables to `private`.<br>
Since they're already dumped in a special section, I suspect you<br>
could go further and mark them with `unnamed_addr`, allowing them to<br>
be merged with each other.<br></blockquote><div><br></div><div>I currently need the names with the non-private linkage for the coverage mapping though - this<br>way I don't end up with multiple function mappings for the ODR functions. But it's probably<br>
possible to use the name values instead of relying on the values of the pointers to the names<br></div><div>to achieve the same result.<br><br></div><div>But then I would also prefer for if all the names in a TU were stored in a a single string -<br>
so that I would be able to get rid of a per-function pointer in the coverage data. <br>That would mean that the profile data record would have to to store a pointer into the string (is that possible?).<br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
4. For the "data" variables, I'm not convinced that changing `external`<br>
variables to `private` is correct. Consider how variables get<br>
merged between translation units. If you have a symbol that's<br>
`weak` in one and `external` (strong) in another, references to the<br>
weak symbol will point at the strong one instead. If you change the<br>
`global` one to `private`, the references to the weak symbol won't<br>
get updated. This doesn't seem like the right behaviour.<br>
<br>
Since the "data" variables are in `llvm.used`, I think we'll end up<br>
with two copies of them (one `weak`, the other `private`). They'll<br>
both point at counters, and it won't be clear which is canonical.<br></blockquote><div><br></div><div>I haven't considered this point before, but this seems to be a real concern.<br></div><div>Unfortunately this will mean that the size impact of private linkage will be very small.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I might be missing something. Can you check if there's a testcase<br>
for that case in the compiler-rt tests (`check-profile`) and the<br>
clang tests (`test/Profile`)? If not, that was an oversight --<br>
please add tests for both test suites (and make sure they pass with<br>
your change).<br></blockquote><div><br></div><div>Such test cases don't exist there, I will add them.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
5. For the "counter" variables, your logic is probably fine. What we<br>
need to be sure of is that the surviving definition of the function<br>
in question uses the *same* counters that the surviving "data"<br>
variable points to.<br></blockquote><div><br></div><div>I'm guessing that there is no way to guarantee that, therefore it would be safer<br></div><div>to just use the same linkage as the data variable.<br></div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
6. I'm surprised no clang tests failed after this change (in<br>
particular, I'd have expected `test/profile/*-linkage.*` to start<br>
failing). If they are failing, can you update them? If they<br>
aren't, can you add test coverage for this?<br>
<br></blockquote><div><br></div><div>Sorry, I forgot to update the tests. <br></div></div><br></div></div>