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

[Rong Xu via Phabricator via llvm-commits]

xur added a comment.

In D133121#3775072 <https://reviews.llvm.org/D133121#3775072>, @hoy wrote:

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

Yes. This is the place that need to be changed. 
Here TI is the invoke. It can have either call-count branch weight metadata (set in 1628), or VP metadata set in  line 878.

We need to set the right branch_weight here -- if there is a VP metadada, we create a compound one. If this is single callcount weight, we replace it with real branch_weight.


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

https://reviews.llvm.org/D133121