<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 13, 2023 at 6:39 PM 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 added a comment.<br>
<br>
In D152399#4498827 <<a href="https://reviews.llvm.org/D152399#4498827" rel="noreferrer" target="_blank">https://reviews.llvm.org/D152399#4498827</a>>, @xur wrote:<br>
<br>
> It seems that we now need to specify "-mllvm -mfs-psi-cutoff=0" to enable the MFS for FSAFDO, if we want to get the same optimization behavior.  This is not good for usage adoption. <br>
> I think if the users want to enable MFS in FSAFDO, they should only need to add -fsplit-machine-functions. They should not need to use ANY extra internal options.<br>
<br>
Curious what happens if mfs is turned on for FSAFDO without `-mfs-psi-cutoff=0`? Does it actually cause regression just because different threshold is used from `PSI->isColdCountNthPercentile(PercentileCutoff, *Count)`?<br></blockquote><div><br></div><div>Using "cold" from PSI is not achieving the same performance as using the "cold" in this file. We tried PSI cold in the very beginning and quickly realized we needed another threshold for MFS.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> I know the newer change was to unify the heuristic code in isColdBlock. But I don't like the change here.<br>
><br>
> I would propose to replace  HasAccurateProfile parameter in isColdBlock with an enum of profileKind (InstruProfile, Sample Profile, or Sample Profile with FSAFDO etc). And we can have different heuristic for different profile kind.<br>
<br>
Yes, querying PSI directly is better.<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D152399/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D152399/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D152399" rel="noreferrer" target="_blank">https://reviews.llvm.org/D152399</a><br>
<br>
</blockquote></div></div>