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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

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

> In D133121#3774815 <https://reviews.llvm.org/D133121#3774815>, @hoy wrote:
>
>> In D133121#3774799 <https://reviews.llvm.org/D133121#3774799>, @xur wrote:
>>
>>> In D133121#3774764 <https://reviews.llvm.org/D133121#3774764>, @hoy wrote:
>>>
>>>> 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.
>>>
>>> AutoFDO is less problematic as branch_weights metadata overwrites VP in the end. (and many VP metadata is already consumed by the prompting in preinliner).
>>>
>>> In FDO, VP metadata overwrites branch_weights -- This is bad as we lost more information in BPI/BFI.
>>> (branch_weights is more important for downstream opt).
>>
>> It looks like same happens to autofdo, i.e, no branch_weights assigned to indirect invokes? https://github.com/llvm/llvm-project/blob/7b81a81d5f9cccb1b091cfc5264bc483b0acc83a/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1628
>>
>> Oh, I guess branch_weight is attached to invokes that are promoted from an indirect invoke, so it is better handled than the FDO case.
>
> the branch weight for invoke is set in line 1724 (https://github.com/llvm/llvm-project/blob/7b81a81d5f9cccb1b091cfc5264bc483b0acc83a/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1724)
>
> line 1628 sets the call's branch-weight (which is a single number, call counts) -- this should be overwritten by line 1724.

Line 1724 is guarded by `!TI->extractProfTotalWeight(TempWeight)` which is non-zero for invokes with !VP?


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

https://reviews.llvm.org/D133121