[PATCH] D54175: [PGO] context sensitive PGO

Xinliang David Li via llvm-commits llvm-commits at lists.llvm.org
Sun Nov 11 14:27:53 PST 2018


Rong, can you re-post the reply using the phabricator interface to avoid
creating two different review threads?

thanks,
David

On Thu, Nov 8, 2018 at 11:55 AM Rong Xu <xur at google.com> wrote:

>
>
> On Wed, Nov 7, 2018 at 10:10 AM David Li via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> davidxl added inline comments.
>>
>>
>> ================
>> Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:618
>> +  // Reserve bit 60-63 for other information purpose.
>> +  FunctionHash &= 0x0FFFFFFFFFFFFFFF;
>> +  if (IsCS)
>> ----------------
>> xur wrote:
>> > davidxl wrote:
>> > > This won't work well -- the non-CS FunctionHash may collide with CS
>> hash. How do you differentiate?
>> > >
>> > > Also doing this for nonCS profiling also breaks the backward
>> compatibility -- old profile with high bits set in hash won't be found
>> anymore. It may also create more hash conflicts.
>> > That is the reason I bump the version number.  I use the version number
>> to tell if this is old Hash format or new Hash format.
>> Say there exists an old version profile with the 60-63 bits set for some
>> functions. When this profile is used, the new compiler won't be able to
>> find those functions.
>>
>> For this we will use one VARIANT bit in header to distinguish.
> AnnotateAllFunction will check if the VARIANT flag is set when reading CS
> profiles.
> For old index profile, this won't be a problem as this bit will not be set.
>
> This could be a problem for old raw profile. As of now, I did not change
> instrumentation run-time. So I could only use FuncHash to tell if this
> regular count or CS count. Setting of VARIANT bit in index profile relies
> on the new FuncHash scheme.
>
> I can add change to profile runtime and set the VARIANT for raw profile.
> Then setting the VARIANT bit for index profile will not rely on check of
> FuncHash. This will fully backward compatible.
>
> The new FuncHash scheme will be guarded by a check of VARIANT bit. Maybe
> we don't need bump the profile version.
>
> Also where is the code that checks the version number?
>>
>
>  Right now, It's only in IndexedInstrProfReader::readSummary().
>
>
>> https://reviews.llvm.org/D54175
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181111/7c36c6bd/attachment.html>


More information about the llvm-commits mailing list