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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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
----------------
wmi wrote:
> 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.
That's a good point. We are using seven bits to represent [0, 100] so that integral numbers can be distinguished. Yes, we could use fewer bits to represent, say 4 bits to represent only even numbers. We could also not use any bits here but instead use the distribution factor of the outer block probes when the competition of those bits are high. I can do an experiment to see how well that works.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:273
   IRChangedPrinter PrintChangedIR;
+  PseudoProbeVerifier PseudoProbeVerification;
   VerifyInstrumentation Verify;
----------------
wmi wrote:
> 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?  
Yeah, there's no need to verify intermediate passes. The verifier pass is just a handy utility that tracks those passes that do code duplication for debugging. Perhaps I should give it a better name like PseudoCloningTracker?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:133-134
+      float PrevProbeFactor = PrevProbeFactors[I.first];
+      if (std::abs(CurProbeFactor - PrevProbeFactor) >
+          DistributionFactorVariance) {
+        if (!BannerPrinted) {
----------------
wmi wrote:
> Why not issue warning/error message when verification fails? That will make enabling the verification in release compiler possible.
The verifier is for debugging only. It doesn't really do any verification. It just helps to track code duplication. Sorry for the naming confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93264