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

Sat Feb 18 01:38:31 PST 2017

On Wed, Feb 8, 2017 at 11:49 AM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Wed, Feb 8, 2017 at 11:43 AM, Rong Xu <xur at google.com> wrote:
>
>>
>>
>> On Tue, Feb 7, 2017 at 6:55 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>>>
>>>
>>> On Mon, Feb 6, 2017 at 10:27 AM, Rong Xu <xur at google.com> wrote:
>>>
>>>>
>>>>
>>>> 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?
>>>>>
>>>>
>>>> Another disadvantage is this is less tolerable to source changes: if
>>>> the use adds another exported symbol in the use-compilation (which has
>>>> nothing to do with another of the existing functions), the suddenly voids
>>>> all the static function's profile.  This is not happening in current path
>>>> name based scheme.
>>>>
>>>
>>> You are arguing that this is a correctness issue, and I gave an example
>>> where the path based scheme is incorrect. Conservatively invalidating
>>> profile info seems preferable, no?
>>>
>>>
>> I'm not sure what you are referring to here. If it is the option of
>> -static-func-full-module-prefix, yes, this is a correctness issue. This
>> generates bad code in instrumentation compilation that seg-fault the binary
>> If you were talking the hash-based uniquely ID for static function, I
>> don't mean  this is a correction issue.
>> On the other hand, I also don't think path based scheme is of correctness
>> issues either (refers to your "incorrect").
>>
>> For both method, the worst case will be drop of the profiles.
>>
>> Also let's look at your example above, function PreprocessHelper has 2
>> implementations under MACRO USE_AVX, and I assume you mean profile-use and
>> profile-generate have different code path because of this.
>> Is't this the same as the user changes the source after profile-generate?
>> This applies to non-static function also.
>> We meant to use function hash to catch this case. IMO, this has little to
>> do with static function naming.
>>
>
>
> What Sean meant is that full path can not resolve conflict 100%. The use
> case of the small example is like:
>
> clang -DUSE_AVX -o test_avx.o  -c /a/b/c/test.c
> clang -o test_noavx.o  -c /a/b/c/test.c
>
> basically same source file producing two different modules.
>

For the record, the only places that i can actually remember seeing this
have been code that compiles the same .c file's once for 32-bit and once
64-bit when handling ELF files. I remembered at least one in FreeBSD, and I
recently just ran into this one:
http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/sgs/libld/common
(
http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/sgs/include/libld.h
Depending on the bit width, it does:
```
#if defined(_ELF64)

#define ld_create_outfile ld64_create_outfile
#define ld_ent_setup ld64_ent_setup
#define ld_init_strings ld64_init_strings
#define ld_init_target ld64_init_target
#define ld_make_sections ld64_make_sections
#define ld_main ld64_main
#define ld_ofl_cleanup ld64_ofl_cleanup
#define ld_process_mem ld64_process_mem
#define ld_reloc_init ld64_reloc_init
#define ld_reloc_process ld64_reloc_process
#define ld_sym_validate ld64_sym_validate
#define ld_update_outfile ld64_update_outfile

#else

#define ld_create_outfile ld32_create_outfile
#define ld_ent_setup ld32_ent_setup
#define ld_init_strings ld32_init_strings
#define ld_init_target ld32_init_target
#define ld_make_sections ld32_make_sections
#define ld_main ld32_main
#define ld_ofl_cleanup ld32_ofl_cleanup
#define ld_process_mem ld32_process_mem
#define ld_reloc_init ld32_reloc_init
#define ld_reloc_process ld32_reloc_process
#define ld_sym_validate ld32_sym_validate
#define ld_update_outfile ld32_update_outfile

#endif
```

It then just uses names like `ld_main` in the source code:
http://src.illumos.org/source/xref/illumos-gate/usr/src/cmd/sgs/libld/common/ldmain.c#144

)

-- Sean Silva

>
>
>>
>> Also what if there are no exported symbols of foo_avx() and foo_noavx().
>> It will generate the same ID in hash-based method.
>>
>>
> One global symbol (data or function) needs to be defined and reference the
> static function to make it live.
>
> David
>
>
>>
>>
>>
>>
>>> -- Sean Silva
>>>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>> -- Sean Silva
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> https://reviews.llvm.org/D29512
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170218/167903c7/attachment.html>