[llvm] r350671 - [PGO] Use SourceFileName rather module name in PGOFuncName
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 22 17:03:21 PST 2019
On Tue, Jan 22, 2019 at 4:10 PM Rong Xu <xur at google.com> wrote:
> On Tue, Jan 22, 2019 at 2:44 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Tue, Jan 22, 2019 at 2:39 PM Rong Xu <xur at google.com> wrote:
>>
>>> On Tue, Jan 22, 2019 at 1:24 PM Teresa Johnson <tejohnson at google.com>
>>> wrote:
>>>
>>>> On Tue, Jan 22, 2019 at 1:06 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>> >
>>>> > Hey Rong, Teresa,
>>>> >
>>>> > This seems like it might be problematic to me - Couldn't multiple
>>>> modules have the same source file name (built with different preprocessor
>>>> defines, etc) - at least I think that's the case for some projects at
>>>> Google.
>>>> >
>>>> > Does this identifier need to be unique? What are the ramifications if
>>>> multiple modules had the same source file name & this situation?
>>>>
>>>> Rong will know for sure, but I think the profiles will be merged and it
>>>> is possible there will be a profile mismatch.
>>>>
>>>> However, note that this is what already happens for PGO - this change
>>>> retains the status quo since PGO is matched during the compile from source
>>>> where the module name (F.getParent()->getName()) is the same as
>>>> F.getParent()->getSourceFileName(). So any uniqueness issue already exists
>>>> with PGO.
>>>>
>>>> This change is to enable context sensitive PGO which must be done in
>>>> the *LTO backends, where we have recorded the original module name in the
>>>> SourceFileName in the bitcode (in the *LTO backends the module name is the
>>>> .o bitcode file name).
>>>>
>>>>
>>> Thanks for Teresa explanation and the background. I just want to add one
>>> more thing: we use both filename and function's CFG checksum in the static
>>> function's PGOFuncName. If multiple functions have the same source name,
>>> but with different CFG checksums, they can coexist in the profile.
>>> Profile-use compilation will pick the exact match one. In theory, there
>>> still exist some collisions, but the chance are very small.
>>>
>>
>> So if this was the original source file name, it wouldn't be too hard to
>> collide, even with the CFG checksum mixed in, I think. In Google, at least,
>> it's not too uncommon (I've seen it a few times, at least) for a source
>> file to be built into multiple libraries, either by mistake or
>> intentionally (if it's intentional, then the builds probably use different
>> parameters - different #defines and the like) - and a file-local static
>> function in such a file might not depend on those #defines or other
>> parameters, so it would be the same across files - while still being
>> distinct functions.
>>
>> For Google applications, we use distributed thinlto build. The patch
> won't affect them as the IR files (with .o suffix) is passed the post LTO
> compilation. This patch will treat the .o as the source files.
>
>
>> But if the filename being used is the name of the intermediate .o file
>> rather than the original source file, well that filename has to be unique
>> for the linker to be able to read both .o files (even if they came from the
>> same source file) & so it should provide a good uniqueness.
>>
>> This only affects the compilation through linker plugin. Without this
> patch, if we call that function after LTO linking, all the static function
> will have a mismatch. As the profile-use and profile-gen have different
> module name (with a unique temporary string in the file name). This is much
> worse than the collision.
>
Ah, OK - so what's the source name if the input to the linker is an IR file
(with .o suffix) as would usually be the case (but with linker plugin
rather than distributed thinlto) - is it still the .o file name passed to
the linekr? or is it now the user source code file name? If it's still the
.o file name, that'd still avoid a collision, I think?
- Dave
>
>
>> - Dave
>>
>>
>>>
>>>
>>>> Teresa
>>>>
>>>> >
>>>> > - Dave
>>>>
>>>> On Tue, Jan 8, 2019 at 2:43 PM Rong Xu via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: xur
>>>>> Date: Tue Jan 8 14:39:47 2019
>>>>> New Revision: 350671
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=350671&view=rev
>>>>> Log:
>>>>> [PGO] Use SourceFileName rather module name in PGOFuncName
>>>>>
>>>>> In LTO or Thin-lto mode (though linker plugin), the module
>>>>> names are of temp file names which are different for
>>>>> different compilations. Using SourceFileName avoids the issue.
>>>>> This should not change any functionality for current PGO as
>>>>> all the current callers of getPGOFuncName() is before LTO.
>>>>>
>>>>> Differential Revision: https://reviews.llvm.org/D56327
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/lib/ProfileData/InstrProf.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=350671&r1=350670&r2=350671&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
>>>>> +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Tue Jan 8 14:39:47 2019
>>>>> @@ -252,11 +252,12 @@ static StringRef stripDirPrefix(StringRe
>>>>> // data, its original linkage must be non-internal.
>>>>> std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t
>>>>> Version) {
>>>>> if (!InLTO) {
>>>>> - StringRef FileName = (StaticFuncFullModulePrefix
>>>>> - ? F.getParent()->getName()
>>>>> - :
>>>>> sys::path::filename(F.getParent()->getName()));
>>>>> - if (StaticFuncFullModulePrefix && StaticFuncStripDirNamePrefix !=
>>>>> 0)
>>>>> - FileName = stripDirPrefix(FileName,
>>>>> StaticFuncStripDirNamePrefix);
>>>>> + StringRef FileName(F.getParent()->getSourceFileName());
>>>>> + uint32_t StripLevel = StaticFuncFullModulePrefix ? 0 :
>>>>> (uint32_t)-1;
>>>>> + if (StripLevel < StaticFuncStripDirNamePrefix)
>>>>> + StripLevel = StaticFuncStripDirNamePrefix;
>>>>> + if (StripLevel)
>>>>> + FileName = stripDirPrefix(FileName, StripLevel);
>>>>> return getPGOFuncName(F.getName(), F.getLinkage(), FileName,
>>>>> Version);
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>>>
>>>> --
>>>> Teresa Johnson | Software Engineer | tejohnson at google.com |
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190122/0f51cb5c/attachment.html>
More information about the llvm-commits
mailing list