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

[Rong Xu via llvm-commits]

On Mon, Jun 21, 2021 at 11:30 AM Wenlei He via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>
OK. This is a fair statement.  Let me fall back to the original state then.
I'll defer this change when we change the default value of
--enable-fs-discrimintor. At that time, we need to have the extra parameter
and set it at the caller.
For now, when --enable-fs-discrminator is default false, we don't need to
worry about this.

>
>
> ================
> 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? :)

No. It's actually NFC in this patch.  This is because this code is
currently only called by ProfileLoader. In ProfileLoader,
getDiscriminator() is the same as getBaseDiscriminator() because only base
discriminator is used.
This only matters for FS-ProfileLoader.


>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D104584/new/
>
> https://reviews.llvm.org/D104584
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210621/b7774cfa/attachment.html>