[PATCH] D29512: [PGO] Directory name stripping in global identifier for static functions

[Teresa Johnson via llvm-commits]

On Fri, Feb 3, 2017 at 5:12 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Fri, Feb 3, 2017 at 4:51 PM, Rong Xu <xur at google.com> wrote:
>
>> ThinLTO is fact coupled with PGO naming scheme as of now. It needs to
>> have the same naming scheme to import the right indirect-call function.
>>
>> We can move this options to InstrProf.cpp, but this will break the
>> indirect-call-promotion in thin-lto mode.
>>
>
> It is already broken with -static-func-full-module-name.  The new
> option's main purpose is to make prof-gen and use consistent, so it belongs
> to PGO only.
>

But if the option is enabled in some fashion (like the current option is
enabled), we will lose ThinLTO indirect call promotion to static functions
since the name won't match. Note we don't actually want to do any path
prefix stripping in ThinLTO mode, since otherwise we will hit other issues
(conflicts in the index). So ideally we won't have these options enabled at
the same time as ThinLTO, but is there a way to detect and enforce that?


>
>> I'm not sure if I understand the reconstruction method. How can this
>> reconstruction affect the importing in thin-lto?
>>
>
> The issue is that after static promotion (static func becomes global), the
> promoted/raw name needs to be used for target lookup -- this is the reason
> why the static promotion currently needs to use the exact same scheme as
> PGO.
>

Which static promotion are you referring to? ThinLTO does not use the PGO
name (aka global identifier) for static promotion. Only as the key to the
combined index. It is this key that needs to match the PGO name for ThinLTO
indirect call promotion to work.

Teresa

What I suggest is that PGO code should recognize the promoted symbol and
> use its original name+linkage or use the PGO name metadata you introduced.
>
> David
>
>>
>>
>>
>> On Fri, Feb 3, 2017 at 4:16 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> On Fri, Feb 3, 2017 at 4:00 PM, Teresa Johnson via Phabricator <
>>> reviews at reviews.llvm.org> wrote:
>>>
>>>> tejohnson added inline comments.
>>>>
>>>>
>>>> ================
>>>> Comment at: lib/IR/Globals.cpp:159
>>>> +    else {
>>>> +      if (StaticFuncStripDirNamePrefix != 0)
>>>> +        FileName = stripDirPrefix(FileName,
>>>> StaticFuncStripDirNamePrefix);
>>>> ----------------
>>>> davidxl wrote:
>>>> > getGlobalIdentifier is a low level interface. Move this into
>>>> getPGOFuncName
>>>> Rong and I discussed this - putting it here ensures the same naming
>>>> scheme is used for PGO and for the ThinLTO index. Although for ThinLTO we
>>>> will likely not want this enabled. I'm not sure if there is a good way to
>>>> enforce that.
>>>>
>>>>
>>> Ideally we should not couple ThinLTO naming scheme with PGO lookup name
>>> scheme -- PGO code should be able to 'reconstruct' the original static
>>> function name and then use getPGOFuncName to form the lookup name -- but
>>> that is for later.
>>>
>>> For now, it is better to isolate this internal option to the domain of
>>> PGO only.
>>>
>>> David
>>>
>>>
>>>
>>>>
>>>> https://reviews.llvm.org/D29512
>>>>
>>>>
>>>>
>>>>
>>>
>>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170203/4226df6b/attachment.html>