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


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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:356
+      if (DIL->getDiscriminator()) {
+        DIL = DIL->cloneWithDiscriminator(0);
+        Probe->setDebugLoc(DIL);
----------------
What if two probes come from the same line? should we still expect a unique line+base_disc to be able to differentiate them? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148569