[PATCH] D104584: [SampleFDO] Make FSDiscriminator flag part of function parameters

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm.



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2203-2210
 unsigned DILocation::getBaseDiscriminator() const {
-  return getBaseDiscriminatorFromDiscriminator(getDiscriminator());
+  return getBaseDiscriminatorFromDiscriminator(getDiscriminator(),
+                                               EnableFSDiscriminator);
+}
+unsigned DILocation::getBaseDiscriminator(bool IsFSDiscriminator) const {
+  return getBaseDiscriminatorFromDiscriminator(getDiscriminator(),
+                                               IsFSDiscriminator);
----------------
xur wrote:
> wmi wrote:
> > Can we just use "unsigned DILocation::getBaseDiscriminator(bool IsFSDiscriminator = false)" or we still need the two API?
> Note that one still checks the option of enable-fs-discrminator. And the other uses a flag (when we know for sure the format).
> If we just keep one, we will probably keep the one without parameters. 
> 
> If we want to use the one with default (false) parameters, we need to set the options in many callers.
> 
This is indeed somewhat redundant. Understand that with two functions, we avoid changing many call sites, but it's probably cleaner to unify (default to false, pass `EnableFSDiscriminator` from callers when needed). I don't have a strong opinion.


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:284
+  if (EnableFSDiscriminator)
+    Discriminator = DIL->getDiscriminator();
+  else
----------------
This seems to be the most important change in the patch? :) 


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

https://reviews.llvm.org/D104584