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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:356
+      if (DIL->getDiscriminator()) {
+        DIL = DIL->cloneWithDiscriminator(0);
+        Probe->setDebugLoc(DIL);
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > What if two probes come from the same line? should we still expect a unique line+base_disc to be able to differentiate them? 
> > We do not need a unique line number for each probe as the probe id is used to determine the probe's discriminator. 
> Sounds good, hope it (not having unique line+disc) will not make merging probes more likely to happen for downstream opts..
Right, it will unlikely affect the merging which doesn't look at debug data anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148569