[PATCH] D147286: [FS-AFDO] Assign discriminators to pseudo probes

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

In D147286#4243873 <https://reviews.llvm.org/D147286#4243873>, @hoy wrote:

> In D147286#4243840 <https://reviews.llvm.org/D147286#4243840>, @wenlei wrote:
>
>> In D147286#4243608 <https://reviews.llvm.org/D147286#4243608>, @hoy wrote:
>>
>>> In D147286#4242407 <https://reviews.llvm.org/D147286#4242407>, @wenlei wrote:
>>>
>>>>> Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.
>>>>
>>>> How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?
>>>
>>> They'll be handled separately. Basically they will be ignored in MIR sample loading.
>>
>> So you will need a separate patch to handle them, right? Without that, they are being ignored MIR sample loading. Perhaps worth clarifying in your change description.
>>
>> My understand is: this is the first (of the two?) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch.
>
> Yes, there will be a separate change for probe integration with MIR sample loading and call probes will be handled there.

Ok, please update the description to clarify. I would add something like "this is the first (of the two) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch."

>>> This is to reduce hash conflicts.
>>
>> May be nitpicking but this can be confusing too. I thought it's not about conflict. When we have probe (i.e. CSSPGO is on), the anchors are probes instead of debug lines, so we just naturally assign FS discriminators on the probe (anchors) instead. Of course, since we have less probes than general instructions, hash collision is less likely to happen.
>
> Right, we don't need to handle dwarf lines if we only want to use probe as the anchors. But handling probes and lines exclusively basically means we cannot do dual profiles for FS-AFDO. I think if hash conflict was not an issue we wouldn't go exclusive.

I see what you mean now, I wasn't thinking about making them co-exist. Again, please make it clear in the change description so people understand it better.

Otherwise lgtm, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147286