[PATCH] D133121: [PGO] Annotate branch_weight for invoke instruction

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D133121#3774657 <https://reviews.llvm.org/D133121#3774657>, @xur wrote:

> In D133121#3772956 <https://reviews.llvm.org/D133121#3772956>, @hoy wrote:
>
>> Thanks for the change. Wondering if you saw performance improvement with it.
>>
>> The corresponding change on the sample profile side isn't included, do you have any plan for it?
>
> Yes. For AutoFDO, we still need some change for the invoke in sampleloader
> The situation is slightly different in AutoFDO. In FDO, we do branch_weight annotation first, then do the VP.
>
> In AutoFDO, we do VP annotation first  and then branch_weight annotation. We need to set the branch_weight metadata where there is already a VP metadata (which is the reverse of FDO).
> I was planning to have a separated patch for AutoFDO, but I can also include it in this patch.

A separate patch sounds good, thanks.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2168
+  ProfileData = InvokeInst::getIndirectCallProfData(ProfileData);
+  if (!ProfileData) {
+    uint64_t TotalWeight;
----------------
xur wrote:
> hoy wrote:
> > Should the check be `if (ProfileData)`?  If the original invoke doesn't have !VP, the new call should end up with no metadata based on the existing behavior.
> If the original invoke does not have VP, it can still have branch_weights.
> 
> Maybe my words in patch description is confusing: this patch tries to fix the case where invoke calls to a indirect function.
> If the argument of invoke is a direct function -- it will only have a branch_weight and we are doing the right annotations.
> 
> For this transformation, mostly like the invoke is a direct function, in which case, we will annotate a total counts -- FDO will not use
> this metadata. AutoFDO might use this in the inliner.
> 
> If here the newcall is an indirect function, we have have two annotations in invoke and can only have one in the new call. I choose to keep the indirect metadata. This changes the semantic but I think it's the right thing to do.
I see, thanks for the explanation.


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

https://reviews.llvm.org/D133121