[CSSPGO] Fix issues with post-link function checksum check

[wlei]

Summary

Previously we used profileIsValid to check if the function checksum is mismatched in profile matching, however I recently found profileIsValid also returns false if the function doesn't have pseudo probe desc, which doesn't match the original intention.
This tried to fix it by checking the checksum directly. Note that it could cause two changes in thin-lto pipeline that might affect the perf or even regress the perf , those are just by-design and non-trivial to fix here, so I sent this first, we can fix them in different patches depending on more perf measurements.

• Imported functions don't have the probe desc, now they will be blocked for stale profile matching.
• During profile annotation, imported functions are blocked to reload/overwrite the profile.

[hoy]

How does perf look like after disabling annotating imported functions?

llvm/lib/Transforms/IPO/SampleProfile.cpp

1825

nit: external -> imported

2279

Can we assert FuncDesc to be non-null here?

[wlei]

Updating D156715: [CSSPGO] Fix issues with post-link function checksum check

[wlei]

In D156715#4550679, @hoy wrote:

How does perf look like after disabling annotating imported functions?

So far from what I tested, there is no perf change if only disabling annotating imported functions. Note that this also disables the profile matching for the imported functions, so it could cause perf issue, depending on if the imported functions is stale.

llvm/lib/Transforms/IPO/SampleProfile.cpp

2279

As I said in other comments, FuncDesc can be null for imported functions, this patch disables the matching for them, we need a new patch to cover this.

[hoy]

In D156715#4558585, @wlei wrote:

In D156715#4550679, @hoy wrote:

How does perf look like after disabling annotating imported functions?

So far from what I tested, there is no perf change if only disabling annotating imported functions. Note that this also disables the profile matching for the imported functions, so it could cause perf issue, depending on if the imported functions is stale.

Discussed offline. This effect of this change is potentially two-fold :

1. positive impact of disabling unnecessary matching for non-stale profiles
2. negative impact of disabling matching for to-be-inlined imported functions with a stale profile.

Currently they are not tested separately. With a solution of #1, allowing matching for #2 might be a win. Suggesting to hold on this change until we have a good solution for #1.

Thoughts?

[wlei]

In D156715#4561996, @hoy wrote:

In D156715#4558585, @wlei wrote:

In D156715#4550679, @hoy wrote:

How does perf look like after disabling annotating imported functions?

So far from what I tested, there is no perf change if only disabling annotating imported functions. Note that this also disables the profile matching for the imported functions, so it could cause perf issue, depending on if the imported functions is stale.

Discussed offline. This effect of this change is potentially two-fold :

1. positive impact of disabling unnecessary matching for non-stale profiles
2. negative impact of disabling matching for to-be-inlined imported functions with a stale profile.

Currently they are not tested separately. With a solution of #1, allowing matching for #2 might be a win. Suggesting to hold on this change until we have a good solution for #1.

Thoughts?

Yeah, as we discussed offline, sounds good to hold on this, will work on a proper solution for #1 or #2 (alternatively we can pass the metadata or a mismatch flag for the imported function)

[wenlei]

In D156715#4562026, @wlei wrote:

In D156715#4561996, @hoy wrote:

In D156715#4558585, @wlei wrote:

In D156715#4550679, @hoy wrote:

How does perf look like after disabling annotating imported functions?

So far from what I tested, there is no perf change if only disabling annotating imported functions. Note that this also disables the profile matching for the imported functions, so it could cause perf issue, depending on if the imported functions is stale.

Discussed offline. This effect of this change is potentially two-fold :

1. positive impact of disabling unnecessary matching for non-stale profiles
2. negative impact of disabling matching for to-be-inlined imported functions with a stale profile.

Currently they are not tested separately. With a solution of #1, allowing matching for #2 might be a win. Suggesting to hold on this change until we have a good solution for #1.

Thoughts?

Yeah, as we discussed offline, sounds good to hold on this, will work on a proper solution for #1 or #2 (alternatively we can pass the metadata or a mismatch flag for the imported function)

Sounds reasonable. Hope we can make progress on #1.

llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h

 } // end namespace afdo_detail
 
 // This class serves sample counts correlation for SampleProfileLoader by
 // analyzing pseudo probes and their function descriptors injected by
 // SampleProfileProber.
 class PseudoProbeManager {
   DenseMap<uint64_t, PseudoProbeDescriptor> GUIDToProbeDescMap;
 
-  const PseudoProbeDescriptor *getDesc(const Function &F) const {
-    auto I = GUIDToProbeDescMap.find(
-        Function::getGUID(FunctionSamples::getCanonicalFnName(F)));
-    return I == GUIDToProbeDescMap.end() ? nullptr : &I->second;
-  }
-
 public:
   PseudoProbeManager(const Module &M) {
     if (NamedMDNode *FuncInfo =
             M.getNamedMetadata(PseudoProbeDescMetadataName)) {
       for (const auto *Operand : FuncInfo->operands()) {
         const auto *MD = cast<MDNode>(Operand);
         auto GUID = mdconst::dyn_extract<ConstantInt>(MD->getOperand(0))
                         ->getZExtValue();
         auto Hash = mdconst::dyn_extract<ConstantInt>(MD->getOperand(1))
                         ->getZExtValue();
         GUIDToProbeDescMap.try_emplace(GUID, PseudoProbeDescriptor(GUID, Hash));
       }
     }
   }
 
+  const PseudoProbeDescriptor *getDesc(const Function &F) const {
+    auto I = GUIDToProbeDescMap.find(
+        Function::getGUID(FunctionSamples::getCanonicalFnName(F)));
+    return I == GUIDToProbeDescMap.end() ? nullptr : &I->second;
+  }
+
   bool moduleIsProbed(const Module &M) const {
     return M.getNamedMetadata(PseudoProbeDescMetadataName);
   }
 
+  bool profileIsHashMismatched(const PseudoProbeDescriptor &FuncDesc,
+                               const FunctionSamples &Samples) const {
+    return FuncDesc.getFunctionHash() != Samples.getFunctionHash();
+  }
+
   bool profileIsValid(const Function &F, const FunctionSamples &Samples) const {
     const auto *Desc = getDesc(F);
     if (!Desc) {
       LLVM_DEBUG(dbgs() << "Probe descriptor missing for Function "
                         << F.getName() << "\n");
       return false;
     }
     if (Desc->getFunctionHash() != Samples.getFunctionHash()) {

llvm/lib/Transforms/IPO/SampleProfile.cpp

 ///
 /// \param F The function to query.
 ///
 /// \returns true if \p F was modified. Returns false, otherwise.
 bool SampleProfileLoader::emitAnnotations(Function &F) {
   bool Changed = false;
 
   if (FunctionSamples::ProfileIsProbeBased) {
-    if (!ProbeManager->profileIsValid(F, *Samples)) {
+    const auto *FuncDesc = ProbeManager->getDesc(F);
+    if (!FuncDesc) {
+      LLVM_DEBUG(dbgs() << "There is no probe_desc for " << F.getName()
+                        << " or it's an imported function."
+                        << "\n");
+      return false;
+    }
+    if (!ProbeManager->profileIsHashMismatched(*FuncDesc, *Samples)) {
       LLVM_DEBUG(
           dbgs() << "Profile is invalid due to CFG mismatch for Function "
                  << F.getName() << "\n");
       ++NumMismatchedProfile;
       if (!SalvageStaleProfile)
         return false;
     }
     ++NumMatchedProfile;

 
 void SampleProfileMatcher::runOnFunction(const Function &F,
                                          const FunctionSamples &FS) {
   bool IsFuncHashMismatch = false;
   if (FunctionSamples::ProfileIsProbeBased) {
     uint64_t Count = FS.getTotalSamples();
     TotalFuncHashSamples += Count;
     TotalProfiledFunc++;
-    if (!ProbeManager->profileIsValid(F, FS)) {
+    const auto *FuncDesc = ProbeManager->getDesc(F);
+    if (FuncDesc && ProbeManager->profileIsHashMismatched(*FuncDesc, FS)) {
       MismatchedFuncHashSamples += Count;
       NumMismatchedFuncHash++;
       IsFuncHashMismatch = true;
     }
   }
 
   std::unordered_set<LineLocation, LineLocationHash> MatchedCallsiteLocs;
   // The value of the map is the name of direct callsite and use empty StringRef