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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for bringing this up - it makes sure we give full context profile precedence even when we had to use context-less profile in prelink.

> When counts are accurate, I was seeing #1 and #2 help reduce code size by preventing post-sample ICP and CGSCC inliner working on obselete metedata.

I assume the above is from CSSPGO, right?

It'd be good to call out in the change description as to why postlink overwrite could be beneficial.

If we target AutoFDO as well, would be good to mention the impact on AutoFDO too.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:226
+    "overwrite-existing-weights", cl::Hidden, cl::init(false),
+    cl::desc("Ignore existing branch weights on IR and always overwrite."));
 namespace {
----------------
nit: keep the blank line before namespace?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1443-1444
             continue;
           // Prorate the callsite counts to reflect what is already done to the
           // callsite, such as ICP or calliste cloning.
           if (FunctionSamples::ProfileIsProbeBased) {
----------------
Btw, this comment is no longer accurate, right? Based on the discussion in D100993, the factor is strictly for handling duplication now, not for scaling counts after ICP. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1466-1467
           }
-          if (!Sum)
-            continue;
+          if (Sum)
+            updateIDTMetaData(I, SortedCallTargets, Sum);
+          else if (OverwriteExistingWeights)
----------------
So we have special case for `Sum == 0` because `updateIDTMetaData` with Sum 0 is reserved for marking promoted targets, right? 

We may also reach `Sum == 0` inside `updateIDTMetaData` after promoted target is removed, however it looks like `annotateValueSite` would simply bailout without removing existing metadata when new target list is empty.. so we may miss some overwrite still? Though for such cases, it needs to be updated even when `OverwriteExistingWeights` is not used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102537