[PATCH] D149597: [FS-AFDO] Do not load non-FS profile in MIR loader.
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 8 18:34:59 PDT 2023
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
More information about the llvm-commits
mailing list