[PATCH] D96932: [SampleFDO] Support enabling -funique-internal-linkage-name

Sat Feb 20 00:07:12 PST 2021

hoy added a subscriber: wlei.
hoy added a comment.

Thanks for working on this. Good to know unique linkage names helps AutoFDO with a 0.5% gain from Google benchmarks.

In D96932#2572160 <https://reviews.llvm.org/D96932#2572160>, @wmi wrote:

>>> Another type of regression is caused by places where we miss to call getCanonicalFnName. Those places are fixed.
>>
>> For context tracker, we have canonicalization done in llvm-profgen as part of profile generation which is why we didn't canonicalize on the compiler side.. Now with the new model that let compiler decide whether to strip suffix based on section flag, we should skip canonicalization llvm-profgen, does that sound right? I guess your profile generation tool doesn't do name canonicalization?
>
> Another usage of canonicalization is to merge profile. For example, suppose we have a function clone from const propagation, and we want to merge the profile from the clone with the profile from its original body. It is easier to handle it through llvm-profgen and make the profile size smaller before feed it into compiler. Our profile generation tool does name canonicalization.

Good point about the cloned functions. I'm wondering if the canonicalization should be uniformly done in the compiler in every place does name mapping, i.e, from Function to Profile or backwards. The profile generator may not have a full view of the IR and it may not strip away the suffixes accurately without seeing the `sample-profile-suffix-elision-policy` attribute.  cc @wlei

================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:774
   static StringRef getCanonicalFnName(StringRef FnName, StringRef Attr = "") {
-    static const char *knownSuffixes[] = { ".llvm.", ".part." };
+    static const char *knownSuffixes[] = {".llvm.", ".part.", ".__uniq."};
     if (Attr == "" || Attr == "all") {
----------------
tmsriram wrote:
> As an aside, where does ".part." come from?
Can you please add a comment about the order of these suffixes? Looks like it's in reversed appending order, i.e., "._uniq." is always the first suffix to the original name. 

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96932/new/

https://reviews.llvm.org/D96932