[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:2311
   assert(!EnableFSDiscriminator && "FSDiscriminator should not call this.");
+  // Do no interfere with pseudo probes.
+  if (isPseudoProbeDiscriminator(getDiscriminator()))
----------------
wenlei wrote:
> perhaps it's worth explaining in the comment, the model used by probe, and why duplication factor isn't needed. 
comment updated.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2312-2313
+  // Do no interfere with pseudo probes.
+  if (isPseudoProbeDiscriminator(getDiscriminator()))
+    return this;
 
----------------
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.





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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148569