[PATCH] D102537: [SampleFDO] Overwrite branch weight annotated in previous pass.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1487
+            I.setMetadata(LLVMContext::MD_prof, nullptr);
           updateIDTMetaData(I, SortedCallTargets, Sum);
         } else if (!isa<IntrinsicInst>(&I)) {
----------------
wmi wrote:
> The second updateIDTMetaData call should be removed?
Good catch, kind of rebasing issue.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1497-1499
+      for (auto &I : BB->getInstList())
+        if (isa<CallInst>(I) || isa<InvokeInst>(I))
+          I.setMetadata(LLVMContext::MD_prof, nullptr);
----------------
wmi wrote:
> Remove MD_prof means compiler may use static heuristic to infer the callsite hotness, which can still be hot. This is different from marking it as cold. 
> 
> If the profile is accurate, why not set the count to 0?
I see. Thanks for pointing it out. We want it to be treated as cold here. Should be using zero count for direct callsites. For indirect callsites, I'm inclined to removing the metadata which shouldn't trigger PGO ICP to do anything. What do you think?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1541-1545
     // Only set weights if there is at least one non-zero weight.
     // In any other case, let the analyzer set weights.
     // Do not set weights if the weights are present. In ThinLTO, the profile
     // annotation is done twice. If the first annotation already set the
     // weights, the second pass does not need to set it.
----------------
wmi wrote:
> Update the comment to mention OverwriteExistingWeights.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102537