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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> 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?



================
Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:297
   Reader->setModule(&M);
-  ProfileIsValid = (Reader->read() == sampleprof_error::success);
+  ProfileIsValid = (Reader->read() == sampleprof_error::success) && (Reader->profileIsFS());
 
----------------
The profile itself isn't really invalid, it's just not suitable for FS loading, so piggyback on ProfileIsValid seems hacky. Suggest to have a more dedicated way to skip. 

Also please add comment to explain why only FS enabled profile is accepted for MIR loading. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149597