[PATCH] D113296: [FS-AFDO][llvm-profgen] Generate profile with FS-AFDO discriminator

[Lei Wang via Phabricator via llvm-commits]

wlei added a comment.

In D113296#3149453 <https://reviews.llvm.org/D113296#3149453>, @xur wrote:

> This looks good to me.

Thanks for reviewing this patch!



================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:93
   std::unique_ptr<ProfileGeneratorBase> Generator;
   if (ProfileIsCS) {
     Generator.reset(new CSProfileGenerator(Binary, SampleCounters));
----------------
wenlei wrote:
> We don't support CS+FS right now, how about error out for now to prevent misuse? 
Good point!


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:41-52
+  static uint32_t getDuplicationFactor(unsigned Discriminator,
+                                       bool IsFSD = IsFSDiscriminator) {
+    return IsFSD ? 1
+                 : llvm::DILocation::getDuplicationFactorFromDiscriminator(
+                       Discriminator);
   }
 
----------------
wenlei wrote:
> In what scenario would use a `IsFSD` parameter different from the `ProfileGeneratorBase::IsFSDiscriminator`? Wondering if we could just omit the parameter and use `IsFSDiscriminator` directly inside.  
Yeah, there is only one exception that calling `getBaseDiscriminator` to decode the discriminator for callsite in `ProfiledBinary::getExpandedContext`. It happens during unwinding where ProfileGeneratorBase::IsFSDiscriminator is not set yet. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:55
 
+  static bool IsFSDiscriminator;
+
----------------
wenlei wrote:
> nit: UseFSDiscriminator
Changed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113296