[PATCH] D93264: [CSSPGO] Introducing distribution factor for pseudo probe.

[Wei Mi via Phabricator via cfe-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/IR/PseudoProbe.h:41
   //  [18:3] - probe id
-  //  [25:19] - reserved
+  //  [25:19] - probe distribution factor
   //  [28:26] - probe type, see PseudoProbeType
----------------
The bits in discriminator is a scare resource. Have you considered using less bits to represent probe distribution factor? I guess it is possible that using a little more coarse grain distribution factor won't affect performance.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:273
   IRChangedPrinter PrintChangedIR;
+  PseudoProbeVerifier PseudoProbeVerification;
   VerifyInstrumentation Verify;
----------------
Before PseudoProbeUpdate pass, there is no need to verify because PseudoProbeUpdate will make distribution factor consistent. PseudoProbeUpdate run in a late stage in the lto/thinlto prelink pipeline, and no many passes need the verification, so what is the major usage of PseudoProbeVerifier?  


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:133-134
+      float PrevProbeFactor = PrevProbeFactors[I.first];
+      if (std::abs(CurProbeFactor - PrevProbeFactor) >
+          DistributionFactorVariance) {
+        if (!BannerPrinted) {
----------------
Why not issue warning/error message when verification fails? That will make enabling the verification in release compiler possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93264