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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

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.

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



================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:134-140
+      if (HasPseudoProbe) {
+        // Only assign discriminators to pseudo probe instructions. Call
+        // instructions are excluded since their dwarf discriminators are used
+        // for other purposes, i.e, storing probe ids.
+        if (!I.isPseudoProbe())
+          continue;
+      } else if (ImprovedFSDiscriminator && I.isMetaInstruction()) {
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > nit: 
> > > > ```
> > > > if ((ImprovedFSDiscriminator && I.isMetaInstruction()) || 
> > > >     (HasPseudoProbe && !I.isPseudoProbe()) {
> > > >   continue;
> > > > }
> > > > ```
> > > Actually we need to go with the probe check first because probe is also a meta instruction.
> > > 
> > > 
> > > 
> > Ok, but when HasPseudoProbe is false, are we still going to have probe instructions? I thought we won't.
> Right, when `HasPseudoProbe` is false we don't have probes. But `ImprovedFSDiscriminator` and `HasPseudoProbe` can be true at the same time, so checking  `ImprovedFSDiscriminator && I.isMetaInstruction())` first will ignore all probe instructions. 
Ok, thanks for clarification. It's a bit tricky..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147286