[PATCH] D148569: [PseudoProbe] Clean up dwarf discriminator and avoid duplicating factor.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2312-2313
+  // Do no interfere with pseudo probes.
+  if (isPseudoProbeDiscriminator(getDiscriminator()))
+    return this;
 
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > since you already guard this at call sites, is this still needed, or should it be an assert? 
> > > > 
> > > > line-based FS-AFDO doesn't use duplication factor either, where is the use of duplication factor prevented for that today?
> > > This is still needed as the guard on the callsite is to filter block probes. The logic here is mainly for callsite probes.
> > > 
> > > > line-based FS-AFDO doesn't use duplication factor either, where is the use of duplication factor prevented for that today?
> > > 
> > > The callsites of this function are disabled at For FS-AFDO, e.g, https://github.com/llvm/llvm-project/blob/5362a0d859d8e96b3f7c0437b7866e17a818a4f7/llvm/lib/Transforms/Utils/LoopUnroll.cpp#L516 .  The logic here is not only for FS-AFDO, but also for in general not screwing up callsite probe ids.
> > > 
> > > 
> > > 
> > > This is still needed as the guard on the callsite is to filter block probes. The logic here is mainly for callsite probes.
> > 
> > Where is the call site for call probes? Why this is guarded in the function for call probe, but at call site for block probe?
> > 
> > I saw three places where this clone is used, and two of the three you guarded them at call site. The remaining one is in VPlan.cpp. Is that inconsistency intentional?
> > 
> Block probe is in form of probe intrinsics, and the checks enforced in this change, i.,e, ` if (!I.isDebugOrPseudoInst())`, excludes probe intrinsics from getting duplicated factor updated.
> 
> Call probe is just attached to user call instruction in form of dwarf discriminator, and they are not excluded at the callsites of this function. We could do that, but I think it's more cleaner to do it here.
> 
> > I saw three places where this clone is used, and two of the three you guarded them at call site. The remaining one is in VPlan.cpp. Is that inconsistency intentional?
> 
> Good catch. Should also exclude probe intrinsic there for completeness, though it doesn't affect anything since FS-AFDO is already excluded there. 
Actually we have to exclude probe there, otherwise the duplication factor will be mistakenly encoded as a FS-discriminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148569