[PATCH] Use private linkage for the profile instrumentation variables that don't need to keep one copy when linking.

[Alex L]

2014-07-24 11:35 GMT-07:00 Alex L <arphaman at gmail.com>:

>
>
>
> 2014-07-23 14:03 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
>
> > On 2014-Jul-23, at 10:57, Alex L <arphaman at gmail.com> wrote:
>> >
>> > Hi,
>> >
>> > 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.
>> >
>> > The private linkage forces the names for those variables to be
>> stripped, reducing the resulting object file size.
>> >
>> > Alex
>> > <PGOprivateLinkage.patch>
>>
>> Thanks Alex, this sounds promising.
>>
>>  1. Can you post numbers for the file size reduction?
>>
>
> Using the original patch I've observed the following reductions:
>
> cmake: -6%
> FileCheck: -11%
> llvm-profdata: -13%
> llvm-tblgen: -14%
> gzip.c: -12.5%
> luac: -28%
> polarssl/ssl_server: -20%
>
>
>>
>>  2. I think it makes sense to change any `internal` symbols to
>>     `private`.
>>
>>  3. I suspect you could change *all* "name" variables to `private`.
>>     Since they're already dumped in a special section, I suspect you
>>     could go further and mark them with `unnamed_addr`, allowing them to
>>     be merged with each other.
>>
>
> I currently need the names with the non-private linkage for the coverage
> mapping though - this
> way I don't end up with multiple function mappings for the ODR functions.
> But it's probably
> possible to use the name values instead of relying on the values of the
> pointers to the names
> to achieve the same result.
>
> But then I would also prefer for if all the names in a TU were stored in a
> a single string -
> so that I would be able to get rid of a per-function pointer in the
> coverage data.
> That would mean that the profile data record would have to to store a
> pointer into the string (is that possible?).
>
>
>>
>>  4. For the "data" variables, I'm not convinced that changing `external`
>>     variables to `private` is correct.  Consider how variables get
>>     merged between translation units.  If you have a symbol that's
>>     `weak` in one and `external` (strong) in another, references to the
>>     weak symbol will point at the strong one instead.  If you change the
>>     `global` one to `private`, the references to the weak symbol won't
>>     get updated. This doesn't seem like the right behaviour.
>>
>>     Since the "data" variables are in `llvm.used`, I think we'll end up
>>     with two copies of them (one `weak`, the other `private`).  They'll
>>     both point at counters, and it won't be clear which is canonical.
>>
>
> I haven't considered this point before, but this seems to be a real
> concern.
> Unfortunately this will mean that the size impact of private linkage will
> be very small.
>

The thing is that the `weak` and `external` symbols can represent functions
with different
source code, and I think for that case it makes sense not to merge the
profile data for
the `weak` function with the profile data for the `external` function.
Therefore I believe
that assigning `private` instead of `external` should work.

However, right now when the raw profile data ends up with two records for
the function
with the same name, the profile data tool isn't able to handle the merging
of them correctly
as the hashes get mismatched because the functions have different source
code.
Thus the profile data tool has to be updated to handle this case in order
for this patch to be correct.


>
>
>>
>>     I might be missing something.  Can you check if there's a testcase
>>     for that case in the compiler-rt tests (`check-profile`) and the
>>     clang tests (`test/Profile`)?  If not, that was an oversight --
>>     please add tests for both test suites (and make sure they pass with
>>     your change).
>>
>
> Such test cases don't exist there, I will add them.
>
>
>>
>>  5. For the "counter" variables, your logic is probably fine.  What we
>>     need to be sure of is that the surviving definition of the function
>>     in question uses the *same* counters that the surviving "data"
>>     variable points to.
>>
>
> I'm guessing that there is no way to guarantee that, therefore it would be
> safer
> to just use the same linkage as the data variable.
>
>
>>
>>  6. I'm surprised no clang tests failed after this change (in
>>     particular, I'd have expected `test/profile/*-linkage.*` to start
>>     failing).  If they are failing, can you update them?  If they
>>     aren't, can you add test coverage for this?
>>
>>
> Sorry, I forgot to update the tests.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140724/d0f58198/attachment.html>