[PATCH] D12715: Reduce PGO Instrumentation binary and profile data size (Patch-1)

[Xinliang David Li via llvm-commits]

On Tue, Sep 8, 2015 at 10:00 PM, Sean Silva <chisophugis at gmail.com> wrote:
> silvas added a comment.
>
> Fundamentally, this approach is not a long-term solution to reducing PGO data size. The correct long-term solution for reducing PGO data size requires passing the binary to llvm-profdata.

This basically goes back to my original proposed solution.  Whether
this solution is longer term or not depends on whether it is
sufficiently effective in reducing size -- the data we have now
confirm that it is indeed effective.

>The approach of using name hashes is also not an incremental step towards the long-term solution. So thinking about it a bit more, I'm on the fence about whether a quick solution like this one even makes sense to do.

I am on the fence too, but leaning towards having this one as the
longer term solution -- it is cleaner and less disruptive. It does not
require bumping up the version numbers. (On the other hand, my
original patch does require version change)

> I am very apprehensive of adding complexity to make this approach work, in particular threading information through many layers, as this is likely to get in the way of future work towards the long-term solution.

I don't see the complexity actually. Other than some refactoring, the
code here is very non-intrusive. I don't think it will get in the way
of 'longer term' solution if it ever became a need to do.

>
> My takeaways from the RFC thread (maybe I am misunderstanding) is that we are still using the same format just with one string vs. another.

yes that is true -- that is why version number is not bumped up.

> We shouldn't need to rev the version and there shouldn't need to be any changes in compiler-rt or llvm.

Note that version is not changed. It is just the version field is now
partitioned to allow more information to be passed around. This is a
very useful feature to have.


>Clang can easily diagnose (as an isolated piece of logic) if the user did the pgo training run with md5 mode enabled but the md5 mode flag was not passed or vice versa (it is easy to tell if input function names are md5 sums or not as a diagnostic heuristic).

If we want to enforce matching options in profile-gen and profile-use,
the mismatched option and profile data should result in hard errors --
current implementation allows it to be caught as a hard error (with
encoding in the profile data).

>It is also inconsistent for llvm-profdata to be able to be passed `-function=foo` for an md5 profile; the user should have to pass (truncated) md5 of the function name.

This is usability and convenience -- not inconsistency.

>
>
> ================
> Comment at: test/Instrumentation/InstrProfiling/profiling_md5hash_key.ll:5-6
> @@ +4,4 @@
> +
> + at __llvm_profile_name_foo = hidden constant [16 x i8] c"5cf8c24cdb18bdac"
> +; CHECK: @__llvm_profile_name_foo = hidden constant [16 x i8] c"5cf8c24cdb18bdac", section "__DATA,__llvm_prf_names", align 1
> + at __llvm_profile_name_bar = hidden constant [17 x i8] c"e413754a191db537\00"
> ----------------
> This is backward from a standard md5 sum in hex. We should maintain the invariant that the resulting symbol name is a substring of a standard md5 sum. Otherwise it is very difficult to explain how to get the hash for a function.
>
> E.g. we should be able to say "the symbol name is effectively replaced by the first half of its md5 sum in hex":


Good catch. That can be fixed.

thanks,

David

>
> ```
> Sean:~ % echo -n 'foo' | md5
> acbd18db4cc2f85cedef654fccc4a4d8
> Sean:~ % echo -n 'foo' | md5 | head -c 16
> acbd18db4cc2f85c
> ```
>
> The symbol name string should be `acbd18db4cc2f85c`.
>
>
> http://reviews.llvm.org/D12715
>
>
>