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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

In D102537#2762431 <https://reviews.llvm.org/D102537#2762431>, @hoy wrote:

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

For future reference, can you make it explicit in the change description: 1) what made this beneficial for CSSPGO (give post-link full context profile precedence over pre-link partial context profile, i.e. be more specific about obsolete metadata); 2) what is the expected impact on AutoFDO.

I think there's another caveat worth calling out, by overwriting all pre-link weights, we would lose the prof metadata adjustment done by prelink opt passes (e.g. prelink loop passes).



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1466-1467
           }
-          if (!Sum)
-            continue;
+          if (Sum)
+            updateIDTMetaData(I, SortedCallTargets, Sum);
+          else if (OverwriteExistingWeights)
----------------
hoy wrote:
> 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.
> 
>  
Ok took another look, i was looking at the wrong overload of annotateValueSite. There's one that would bail out without setting metadata if value data is empty, but that's not the overload we call from updateIDTMetaData. So we should be good. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102537