[PATCH] D92347: [CSSPGO] Consume pseudo-probe-based AutoFDO profile

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:554-556
+      // A missing entry for a probe likely means the probe was not executed.
+      // Treat it as a zero count instead of an unknown count to help edge
+      // weight inference.
----------------
I guess it makes sense to treat missing entry to have zero count instead of unknown because the function profiles with unmatched CFGchecksum has been dropped, is that right? 

If it is, can we check CFGchecksum not zero here instead of ProfileIsProbeBased or ProfileIsCS? When CFGchecksum is not zero and matched, we are confident the profile is accurate no matter what kind of profile it is. 

Another thing is: I think debug info based profile can have a similar checksum then it can use a similar strategy here as probe based profile. It may not be a CFGchecksum because debug info based profile has no idea about CFG, so it is better to rename CFGchecksum to something more general.   


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:675-677
+    if (FunctionHash == 0) {
+      // Set the function hash code for the target profile.
+      FunctionHash = Other.getFunctionHash();
----------------
Is it correct that for probe based profile all the FunctionHash should already be initialized to a non-zero value?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:681-682
+      // either:
+      // 1. They are same-named static functions from different compilation
+      // units, or
+      // 2. They are really the same function but from different compilations.
----------------
Item1 won't be an issue if -funique-internal-linkage-names becomes default (https://reviews.llvm.org/rGad1b9daa4bf40c1907794fd5de7807aad1f0553c). 


================
Comment at: llvm/lib/IR/PseudoProbe.cpp:37-43
+        PseudoProbe Probe;
+        Probe.Id =
+            PseudoProbeDwarfDiscriminator::extractProbeIndex(Discriminator);
+        Probe.Type =
+            PseudoProbeDwarfDiscriminator::extractProbeType(Discriminator);
+        Probe.Attr = PseudoProbeDwarfDiscriminator::extractProbeAttributes(
+            Discriminator);
----------------
Can you wrap them into a function like extractProbeFromDiscriminator?


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:135
 /// Print the samples collected for a function on stream \p OS.
 void FunctionSamples::print(raw_ostream &OS, unsigned Indent) const {
   OS << TotalSamples << ", " << TotalHeadSamples << ", " << BodySamples.size()
----------------
Should we output CFGchecksum if it is supported? It will affect the output of 'llvm-profdata show -sample'


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:870
+std::error_code SampleProfileReaderExtBinaryBase::readFuncMetadata() {
+  if (ProfileIsProbeBased) {
+    for (unsigned I = 0; I < Profiles.size(); ++I) {
----------------
Nit: if (!ProfileIsProbeBased) return sampleprof_error::success;


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:171
+    const StringMap<FunctionSamples> &Profiles) {
+  if (FunctionSamples::ProfileIsProbeBased) {
+    auto &OS = *OutputStream;
----------------
Nit: if (!FunctionSamples::ProfileIsProbeBased) return sampleprof_error::success;


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:174
+    for (const auto &Entry : Profiles) {
+      writeNameIdx(Entry.first());
+      encodeULEB128(Entry.second.getFunctionHash(), OS);
----------------
What about the case that the profile use MD5 to represent name?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:684-686
+    assert(!ProfileIsProbeBased ||
+           ProfileIsProbeBased == FunctionSamples::ProfileIsProbeBased);
+    ProfileIsProbeBased = FunctionSamples::ProfileIsProbeBased;
----------------
This should use exitWithError instead of assertion for user to get the error message in release mode, same as you did for compare below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92347