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

Tue Feb 7 20:06:06 PST 2017

On Tue, Feb 7, 2017 at 7:47 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Tue, Feb 7, 2017 at 7:40 PM, Teresa Johnson <tejohnson at google.com>
> wrote:
>
>>
>>
>> On Tue, Feb 7, 2017 at 7:34 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Feb 7, 2017 at 7:25 PM, Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Feb 7, 2017 at 7:19 PM, Xinliang David Li <davidxl at google.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, Feb 7, 2017 at 6:57 PM, Sean Silva <chisophugis at gmail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Feb 6, 2017 at 9:54 AM, Rong Xu <xur at google.com> wrote:
>>>>>>
>>>>>>> When I wrote IRPGO for PGOFuncName, I intentionally chose to use the
>>>>>>> source path with directory name. This is different from the Clang PGO where
>>>>>>> the base name is used. I believe I mentioned this in the comment or commit
>>>>>>> message. This is to avoid the name clashing like this.
>>>>>>>
>>>>>>> So the change in D22028 was a regression.
>>>>>>>
>>>>>>> Surely, this will not solve all the name clashing: if the user could
>>>>>>> have used the same source name string (with directory) in the build. In
>>>>>>> this case, there still could be name clashing. But I don't think it is
>>>>>>> common. I haven't encountered one in the real applications. On the other
>>>>>>> hand, using the base name only breaks many of out applications.  In
>>>>>>> addition, no matter how complicate the scheme is, like Hash, there
>>>>>>> is still a chance for the naming clash (unless, of course, using
>>>>>>> global module ID in thin-lto or lto).
>>>>>>>
>>>>>>
>>>>>> So from what you're saying it seems like we already have a global
>>>>>> module ID that could be used to disambiguate static functions? Why don't we
>>>>>> use that?
>>>>>>
>>>>>
>>>>> It is using the same scheme as PGO.
>>>>>
>>>>
>>>> I thought ThinLTO hashed the exported symbols? (sorry, I guess I'm out
>>>> of the loop)
>>>>
>>>
>>> For function importing, the same scheme is used as PGO.  Regarding
>>> global Module Id, do not take my word for it :) I will let Teresa/Medhi
>>> comment on it.
>>>
>>
>> We use the same name scheme as PGO as the key in the ThinLTO combined
>> index for importing - this is strategic so that we can enable importing of
>> profiled hot indirect call targets which are encoded in the profile with
>> the PGO name scheme.
>>
>> We use a hash of the module's IR for something else, namely for creating
>> a unique name when promoting statics to global scope when necessary.
>>
>
> Full IR or  just exported symbols? For caching purpose, it would need to
> be IR.
>

Right, full IR.
Teresa

>
> David
>
>>
>> Thanks,
>> Teresa
>>
>>
>>> Thanks,
>>>
>>> David
>>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>> Figuring out how many paths to strip from the source dir path seems
>>>>>> like a pain that we shouldn't push on users if we already have a solution.
>>>>>> We want PGO to be as easy to use as possible.
>>>>>>
>>>>>
>>>>> The idea is that most users (who do not generate source into different
>>>>> dirs or use relative source paths) never need to use/worry about it.
>>>>>
>>>>
>>>> Okay. I'll take your word on that for now.
>>>>
>>>> -- Sean Silva
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> -- Sean Silva
>>>>>>
>>>>>>
>>>>>>> I still think using a path name is a good choice.
>>>>>>>
>>>>>>> Second, I don't think D22028's use case is common. The typical use
>>>>>>> case for FDO is to have the same command line options and just replacing
>>>>>>> -fprofile-generate with -fprofile-use. D22028 has different source string,
>>>>>>> I believe the user has a better position to deal with this than the
>>>>>>> compiler.
>>>>>>>
>>>>>>> -Rong
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Feb 5, 2017 at 12:11 AM, Xinliang David Li <
>>>>>>> davidxl at google.com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Feb 3, 2017 at 11:28 PM, Sean Silva <chisophugis at gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, Feb 3, 2017 at 10:18 PM, Xinliang David Li <
>>>>>>>>> davidxl at google.com> wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Fri, Feb 3, 2017 at 8:31 PM, Sean Silva via Phabricator <
>>>>>>>>>> reviews at reviews.llvm.org> wrote:
>>>>>>>>>>
>>>>>>>>>>> silvas added a comment.
>>>>>>>>>>>
>>>>>>>>>>> This change does two things (as you mention in the description):
>>>>>>>>>>>
>>>>>>>>>>> 1. Adding -static-func-strip-dirname-prefix which provides a
>>>>>>>>>>> way to have more control when `-static-func-full-module-prefix=true`
>>>>>>>>>>> is specified.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This is actually a more general form of
>>>>>>>>>> -static-func-full-mdoule-prefix.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 2. Changing the default of -static-func-full-module-prefix to
>>>>>>>>>>> true.
>>>>>>>>>>>
>>>>>>>>>>> IIRC, -static-func-full-module-prefix defaults to false because
>>>>>>>>>>> it caused issues when set to true (in fact, it was introduced to avoid
>>>>>>>>>>> these issues). The default value of -static-func-strip-dirname-prefix
>>>>>>>>>>> introduced in this patch (i.e. 0) is effectively a no-op; so ignore 1. for
>>>>>>>>>>> now. This means that the net effect of this patch is that compilation will,
>>>>>>>>>>> by default, have a regression on the issue fixed by r275193 /
>>>>>>>>>>> http://reviews.llvm.org/D22028, which is not a good idea. I
>>>>>>>>>>> think that the default behavior (which is user-visible) should not be
>>>>>>>>>>> changed in this patch.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I disagree. The original default behavior was to preserve the
>>>>>>>>>> full path which was also user visible :)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> And yet we found a compelling-enough use case to change it.
>>>>>>>>>
>>>>>>>>
>>>>>>>> It was probably better to introduce the option but not flipping the
>>>>>>>> default the first time. The stripping-path-fully mode had not been widely
>>>>>>>> tested at that time.
>>>>>>>>
>>>>>>>> I am a little curious about the use case for D22028. The pgo name
>>>>>>>> of static function is only affected by source module path. Why would that
>>>>>>>> be different for pgo-gen/use builds? In most common setup I saw, the source
>>>>>>>> paths should remain the same.
>>>>>>>>
>>>>>>>>
>>>>>>>>> We may need to revisit that decision, but clearly the current
>>>>>>>>> default is intentional and part of changing away from that is explaining
>>>>>>>>> why we no longer care about that use case (or care about it less than some
>>>>>>>>> other thing).
>>>>>>>>>
>>>>>>>>
>>>>>>>> We care about all use cases, which is why the more general form of
>>>>>>>> option is introduced -- it makes sure the use case in D22028 can also be
>>>>>>>> handled but more safely (stripping all prefix will bound to cause problems).
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The whole rationale for changing the current default is that it
>>>>>>>>>> is generally not safe -- mainly problem #1 because of counter variables for
>>>>>>>>>> static functions can not guaranteed to be unique when full path is stripped.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Can the counter variables be static to match the static nature of
>>>>>>>>> the functions they describe? (there would still be collisions when indexing
>>>>>>>>> the profile data though; the function CFG hash could be included in the
>>>>>>>>> "name" to avoid this)
>>>>>>>>>
>>>>>>>>
>>>>>>>> For simple functions, cfg hash collision is also very likely, so
>>>>>>>> the first line of defense is always the name key.
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> The ThinLTO issue is secondary (probably irrelevant here because
>>>>>>>>>> of other bugs).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The issue addressed in D22028 is actually not common -- the
>>>>>>>>>> source module paths should generally match in profile-gen and profile-use
>>>>>>>>>> phases, so using internal option for that use case seems more reasonable to
>>>>>>>>>> me.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is subjective, but I think it is quite reasonable to assume
>>>>>>>>> that each build will use a different output directory. Hence any build that
>>>>>>>>> generates .cpp files into the output directory (which seems reasonable too)
>>>>>>>>> is susceptible.
>>>>>>>>>
>>>>>>>>
>>>>>>>> so the case is for generated source files? Should they be accessed
>>>>>>>> with relative paths?
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Overall, requiring a user to use a compiler-internal option for
>>>>>>>>> something that seems to happen in practice (e.g. back when I was a
>>>>>>>>> PlayStation we actually ran into it and spent time fixing it) is a pretty
>>>>>>>>> poor experience. I think we should aim to do better (though we might settle
>>>>>>>>> for less if that proves challenging).
>>>>>>>>>
>>>>>>>>
>>>>>>>> I agree in general. However I think it is reasonable for a user to
>>>>>>>> use an internal option for corner use cases.  Another choice is to
>>>>>>>> introduce an external option for this which user can rely on.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Overall, it sounds like this approach of relying on users to
>>>>>>>>>>> tweak internal compiler options (-mllvm) to get correct behavior in their
>>>>>>>>>>> environment is not the kind of user experience we want to deliver (or the
>>>>>>>>>>> kind of implementation that we want to maintain). IIRC, when we added
>>>>>>>>>>> -static-func-full-module-prefix, it was with the understanding
>>>>>>>>>>> that it was a simple hack for working around the larger issue of relying on
>>>>>>>>>>> the module name which we knew was not very robust. The further addition of
>>>>>>>>>>> the "InLTO" complicates things even further. It seems like a code smell
>>>>>>>>>>> that we do not have a Single Point Of Truth.
>>>>>>>>>>>
>>>>>>>>>>> I proposed a solution at one point
>>>>>>>>>>> https://groups.google.com/d/msg/llvm-dev/s_VZbFTWbVs/d0b4Zh8
>>>>>>>>>>> 0CgAJ though it may no longer be applicable. It seems like
>>>>>>>>>>> ThinLTO already has to solve a problem of finding unique identifiers for
>>>>>>>>>>> all functions (even static), so we may want to piggy-back on that mechanism
>>>>>>>>>>> (this is just a high-level thought; haven't looked into the details).
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> For LTO/ThinLTO,  we solved the issue by using meta data which
>>>>>>>>>> uses getPGOFuncName as singe source of truth.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> So:
>>>>>>>>>>>
>>>>>>>>>>> - I specifically object to changing user-visible defaults in
>>>>>>>>>>> this patch. Those changes should be isolated, and I don't think we have
>>>>>>>>>>> justification to change those defaults anyway.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> See my reply about the safety issue of keeping the current
>>>>>>>>>> default.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> - I'm slightly opposed to adding the
>>>>>>>>>>> -static-func-strip-dirname-prefix flag, since it seems like a
>>>>>>>>>>> workaround (among others that have already piled up) for a more fundamental
>>>>>>>>>>> issue. This is a frog-in-boiling-water situation; if solving the
>>>>>>>>>>> fundamental issue would be a huge amount of work, then adding the new flag
>>>>>>>>>>> is probably fine for now, but we need to keep in mind the larger situation.
>>>>>>>>>>> IIUC, defaulting `-static-func-strip-dirname-prefix=-1` would
>>>>>>>>>>> emulate the current default behavior, so -static-func-full-module-prefix
>>>>>>>>>>> could just be removed in the same patch.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The current -static-func-full-module-path=false is simply a
>>>>>>>>>> special case of the new option. For users who rely on this option may hit
>>>>>>>>>> the correctness issue, they won't have any fallback without the new option.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> - I would encourage brainstorming/discussion of alternative
>>>>>>>>>>> solutions that solve the fundamental problem (which seems to be more about
>>>>>>>>>>> having a stable globally unique identifier than being specifically about
>>>>>>>>>>> preserving/mangling the "name" per se).
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The problem itself is simple: handle name conflicts between
>>>>>>>>>>
>>>>>>>>>> /a/b/c/foo.c:static_func
>>>>>>>>>> /e/f/g/foo.c:static_func
>>>>>>>>>>
>>>>>>>>>> Path info is a natural choice. Note that FE instrumentation also
>>>>>>>>>> uses module path to uniquely identify static_func as well.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes, the problem was inherited from FE instrumentation. I remember
>>>>>>>>> that when I explained to Justin the issue, he said that it was clearly
>>>>>>>>> buggy and not intentional (an oversight when implementing FEPGO).
>>>>>>>>>
>>>>>>>>
>>>>>>>> It works very well in practice -- though it is not guaranteed to be
>>>>>>>> 100% free of conflict. I won't label it as buggy.
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> It seems that the fundamental issue is coming up with a unique
>>>>>>>>> identifier for the current TU that is stable across compiler invocations.
>>>>>>>>> How do other compilers handle this?
>>>>>>>>>
>>>>>>>>> GCC does not suffer from the problem because it does not dump
>>>>>>>> profile into one file but one profile file per module. The profile data
>>>>>>>> file tree structure mirrors the build output file structure so there will
>>>>>>>> be problem if profile-gen and use do not share the same structure. Runtime
>>>>>>>> options are provided to strip prefixes from output directories. Compiler
>>>>>>>> time option is also provided to relocate profile data (e.g. pointing to
>>>>>>>> different root).
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> For example, path names are not enough. E.g. a user may build
>>>>>>>>> /a/b/c/foo.c with two different sets of compiler options, yet static
>>>>>>>>> functions of the same name must still be treated as separate. A file like:
>>>>>>>>>
>>>>>>>>> foo.c:
>>>>>>>>>
>>>>>>>>> static void PreprocessHelper(/* something */) {
>>>>>>>>> #ifdef USE_AVX
>>>>>>>>>   // something
>>>>>>>>> #else
>>>>>>>>>   // something else
>>>>>>>>> #endif
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> #ifdef USE_AVX
>>>>>>>>> void foo_avx(/* something */) {
>>>>>>>>> #else
>>>>>>>>> void foo_noavx(/* something */) {
>>>>>>>>> #endif
>>>>>>>>>   PreprocessHelper(/* something */);
>>>>>>>>>   // something
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> IIRC, one option (suggested by pcc if I remember correctly) is to
>>>>>>>>> use a hash of the TU's exported symbols (or something like that) to
>>>>>>>>> uniquely identify the TU.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Should be limited to user defined public symbols.
>>>>>>>>
>>>>>>>>
>>>>>>>>> That seems more robust than a path name.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I believe so.
>>>>>>>>
>>>>>>>> Content based ID has its advantage but has disadvantages too. For
>>>>>>>> instance more expensive to compute, less readable names. Using path based
>>>>>>>> naming, we can immediately identify where the static function is defined.
>>>>>>>> Perhaps we can use base name plus content hash.
>>>>>>>>
>>>>>>>> Teresa, Rong, do you see a situation when  module ID needs to be
>>>>>>>> identified but it is difficult or  too expensive to access the module's
>>>>>>>> content?
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>
>>>>>>>>> -- Sean Silva
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> https://reviews.llvm.org/D29512
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>> --
>> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>> 408-460-2413 <(408)%20460-2413>
>>
>
>

-- 
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/20170207/78c5e9ae/attachment.html>