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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

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

> In D96932#2586378 <https://reviews.llvm.org/D96932#2586378>, @wenlei wrote:
>
>> Actually we are not canonicalizing the names in profile from compiler side, right? We are only canonicalizing names from IR to match names from profile.
>
> That is right.
>
>> Thinking about it more, doing canonicalization in compiler for CSSPGO may be trickier, because we try to reuse a single string for a context. It's feasible to canonicalize context strings in compiler, but it will have cost.
>>
>>> One kind of regression is when we flip -funique-internal-linkage-name on, the profile is collected from binary built without -funique-internal-linkage-name so it has no uniq suffix, but the IR in the optimized build contains the suffix. This may introduce transient regression.
>>
>> This could happen when we migrate. To tackle this one, SecFlagUniqSuffix added in this patch is good enough and we don't need to canonicalize names in profile from compiler side, as they can still be done in the profile generation tools.
>>
>>> In rare case, user may also want to disable -funique-internal-linkage-name after it has been enabled for a while, to triage something for example. In that case, profile will contain the suffix but IR in the optimized build won't.
>>
>> How is this handled currently? Also to me this feel not so important and it's more like debugging helper. If we don't intend to support such case, I don't see a need to move all name canonicalization from profile generation tool into the compiler.
>
> Thanks for pointing it out. I found I made a wrong declaration about it. I mistakenly thought the profile name was canonicalized, and even that is true, it is still tricky to support this scenario. Sorry about the wrong statement. But hopefully the case is rare enough and we don't need to support it too.
>
> The need to move canonicalization to compiler is that profile generation tool cannot see the function attribute. An example is if the profile is generated from a go/c++ mixed code, go may have different canonicalization rule than c++. To be consistent with compiler, the profile generation tool needs to know which symbol is a go symbol and which is a c++ symbol. I don't know whether it is always easy to tell just from the name pattern. Currently it is still not an issue for us (go team did some work on autofdo support before but is not actively working on the support now), but I don't know whether it is needed for us in the future.

Thanks for the clarification. In that case, agreed that we don't need to support the 2nd case of mismatch. For profile name canonicalization inside compiler, since it will have cost for csspgo, I'd suggest we leave it in llvm-profgen (we don't strip unique suffix there, but still handle others, i.e. getCanonicalFnName(.., "selected")) until the need becomes more substantial.. This change isn't doing that for either afdo or csspgo, so looks good on that front.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932