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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D102537#2761666 <https://reviews.llvm.org/D102537#2761666>, @wenlei wrote:

> 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.

Yes, they are for CSSPGO.

> 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.



================
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 {
----------------
wenlei wrote:
> nit: keep the blank line before namespace?
done.


================
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) {
----------------
wenlei wrote:
> 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. 
Good point. The proration is done based on a pre-ICP distribution factor.


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

Right,  `Sum == 0` for `updateIDTMetaData` is used to mark a certain target to be promoted already.

> 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.

This is true. When `Sum` reaches zero when promoted target counts are withdrawn, instead of removing the existing metadata, the current implementation just assigns the call instruction a new zero metadata. That should give equivalent information to subsequent passes.

 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102537