[PATCH] D149597: [FS-AFDO] Do not load non-FS profile in MIR loader.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D149597#4332879 <https://reviews.llvm.org/D149597#4332879>, @wenlei wrote:

> In D149597#4330531 <https://reviews.llvm.org/D149597#4330531>, @hoy wrote:
>
>> In D149597#4328418 <https://reviews.llvm.org/D149597#4328418>, @wenlei wrote:
>>
>>>> I was seeing a regression when enabling FS discriminators on an non-FS CSSPGO build. This is because a probe can get a zero-valued discriminator at a specific pass and that could lead to accidentally loading the corresponding base counter in the non-FS profile
>>>
>>> For non-FS profile, probe doesn't have discriminator, is there no way to differentiate zero discriminator from no discriminator? If a probe in the profile doesn't have discriminator, it should not be considered for FS profile loading. In this case, if input profile is not FS, no probe would have discriminator, hence nothing should be loaded in MIR. Is such checking missing on per-probe level?
>>
>> The body sample map uses id+discriminator as the key, so currently we don't have a way on the function level to tell if a function profile is FS or not. We can tell on the program level, basically using the FS flag.
>
> So the situation here is that the input profile isn't FS, but the current build has FS flag on. Now at MIR profile loading, we get the line+FSdiscriminator from IR, and trying to look up `BodySamples` map via `findSamplesAt`. How does that end up with base profile given the `findSamplesAt` has line+FSdiscriminator as key, which is from DIL on IR. When input profile is not FS, I assume the difference (comparing to when input profile is FS) is that the matching line+FSdiscriminator won't exist in `BodySamples`, hence it will find and load nothing for that location?

Yes, most of the cases the lines will end up loading nothing, and the sample loader doesn't do anything further in terms of annotation and inference (https://github.com/llvm/llvm-project/blob/516e301752560311d2cd8c2b549493eb0f98d01b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h#L877). However for lines that do have a zero FSdiscriminator, line+FSdiscriminator can accidentally match the base counter while all other lines still load nothing. This creates a situation only a few blocks in a function have samples from their base counter while all other blocks will have a zero count. Inferencing from there will be problematic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149597/new/

https://reviews.llvm.org/D149597