[PATCH] D86499: [CSSPGO] Pseudo probe instrumentation pass

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked an inline comment as done.
hoy added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h:36
+/// Insert pseudo probes for block sampling and value sampling.
+class SampleProfileProber {
+public:
----------------
wmi wrote:
> Will the class be used in files other than SampleProfileProbe.cpp? If not, it is better to be placed in SampleProfileProbe.cpp. 
Yes, it will be used outside SampleProfileProbe.cpp. It'll later on introduce a field storing the name of IL metadata that'll be used by later probe emission pass.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:73-76
+    Instruction *J = &*BB->getFirstInsertionPt();
+    while (J != BB->getTerminator() && !HasValidDbgLine(J)) {
+      J = J->getNextNode();
+    }
----------------
wmi wrote:
> Can we add a stat support here to see how frequent the probe cannot find an instruction with valid dbgline?  Even if no instruction with valid dbgline can be found, is it possible to create a dbgline for it and set the line number to 0 or to the function start (I assume line number is of no use. Only the inline context matters here)? 
This is a very good suggestion, thanks! 

The prober pass had not been placed in the very beginning of the pipeline initially and we saw quite some instructions without a real debug line number. It is now placed in the beginning but with some statistics I was still seeing that happened to a few instructions. Giving an artificial line number to such probes will help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86499