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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

>> If we target AutoFDO as well, would be good to mention the impact on AutoFDO too.
>
> I haven't tested for non-CS case. In theory it should help there but with a less-than-ideal counts quality, the change may not help in practice.

AutoFDO doesn't have the distribution factor which CSSPGO+pseudo hook have so OverwriteExistingWeights may not help for AutoFDO.



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


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


================
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.
----------------
Update the comment to mention OverwriteExistingWeights.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102537