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

[Hongtao Yu via Phabricator via llvm-commits]

hoy accepted this revision.
hoy added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:1077
+  InvokeVals.push_back(MDNode::get(Ctx, Vals));
+  MDNode *NewM = MDNode::getDistinct(Ctx, InvokeVals);
+  NewM->replaceOperandWith(0, NewM);
----------------
xur wrote:
> hoy wrote:
> > Can we use `get` instead of  `getDistinct` so that  invokes with same metadata can potentially share the same node?
> Yes. get() seems to be better suited here.
LGTM after this is fixed.


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

https://reviews.llvm.org/D133121