[PATCH] D81981: [PGO] Supplement PGO profile with Sample profile

[Wei Mi via Phabricator via llvm-commits]

wmi marked 4 inline comments as done.
wmi added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1239
     ValueSum += CountFromProfile[I];
+    if (CountFromProfile[I] != (uint64_t)-1)
+      AllMinusOnes = false;
----------------
davidxl wrote:
> Is it possible to have some blocks -1?
I feel having all blocks -1 to indicate unpresentative profile for an actually hot function is simpler than having some blocks -1. That is because when we compute profile summary, we want to strip those unpresentative profile. If we change some blocks to -1 but keep the rest unchanged, those counters will still be used for computing profile summary.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1647
     }
+    // When AllMinusOnes is true, it means the profile for the function
+    // is unrepresentative and this function is actually hot. Set the
----------------
davidxl wrote:
> oh, just move this comment to the variable decl.
Ok, will do. 


================
Comment at: llvm/test/tools/llvm-profdata/Inputs/mix_instr.proftext:11
+
+goo
+5
----------------
davidxl wrote:
> Do we have a test case  for all zero case and below the threshold case (considiered all zero)?
Yes, foo is intentionally created for that. The line 22 and line 42 in suppl-instr-with-sample.test test the cases where ratio of zero counter in foo is above or lower the threshold.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:424
+    // above hot threshold.
+    for (size_t I = 0; I < ProfRecord->Counts.size(); ++I)
+      ProfRecord->Counts[I] = -1;
----------------
davidxl wrote:
> Is it possible to delete the instprof record for the function from the profile?
One reason that I use all -1 as the indication that the function is hot and its profile is unpresentative is: user may build unrelated target together with the PGO optimized target in the same command. I know a lot of SampleFDO user does that to simplify their release. I imagine there could be PGO user doing that too. Another possiblity is to build test targets using the profile.  If we delete the instprof record and treat all the functions without instprof to be hot during prof-use, we may accidently treat a lot of cold functions to be hot if the profile is applied on some unrelated targets or tests (tests may be partially related targets), and that may cause compile-time issue. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D81981