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

[Alex L]

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.


>
>     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/952267a5/attachment.html>