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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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:
> > 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?



================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:356
+      if (DIL->getDiscriminator()) {
+        DIL = DIL->cloneWithDiscriminator(0);
+        Probe->setDebugLoc(DIL);
----------------
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..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148569