<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 21, 2021 at 11:30 AM Wenlei He via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">wenlei accepted this revision.<br>
wenlei added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
lgtm.<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2203-2210<br>
 unsigned DILocation::getBaseDiscriminator() const {<br>
-  return getBaseDiscriminatorFromDiscriminator(getDiscriminator());<br>
+  return getBaseDiscriminatorFromDiscriminator(getDiscriminator(),<br>
+                                               EnableFSDiscriminator);<br>
+}<br>
+unsigned DILocation::getBaseDiscriminator(bool IsFSDiscriminator) const {<br>
+  return getBaseDiscriminatorFromDiscriminator(getDiscriminator(),<br>
+                                               IsFSDiscriminator);<br>
----------------<br>
xur wrote:<br>
> wmi wrote:<br>
> > Can we just use "unsigned DILocation::getBaseDiscriminator(bool IsFSDiscriminator = false)" or we still need the two API?<br>
> Note that one still checks the option of enable-fs-discrminator. And the other uses a flag (when we know for sure the format).<br>
> If we just keep one, we will probably keep the one without parameters. <br>
> <br>
> If we want to use the one with default (false) parameters, we need to set the options in many callers.<br>
> <br>
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.<br></blockquote><div>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.</div><div>For now, when --enable-fs-discrminator is default false, we don't need to worry about this.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
================<br>
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:284<br>
+  if (EnableFSDiscriminator)<br>
+    Discriminator = DIL->getDiscriminator();<br>
+  else<br>
----------------<br>
This seems to be the most important change in the patch? :)</blockquote><div>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.</div><div>This only matters for FS-ProfileLoader.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br>
<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D104584/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D104584/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D104584" rel="noreferrer" target="_blank">https://reviews.llvm.org/D104584</a><br>
<br>
</blockquote></div></div>