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

Tue Feb 7 19:47:31 PST 2017

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.

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/ms
>>>>>>>>>> g/llvm-dev/s_VZbFTWbVs/d0b4Zh80CgAJ 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>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170207/d9bf2ce1/attachment.html>