[PGO] Fix handling of cold entry count for instrumented PGO

[tejohnson]

Summary

In r277849, getEntryCount was changed to return None when the entry
count was 0, specifically for SamplePGO where it means no samples were
recorded. However, for instrumentation PGO a 0 entry count should be
returned directly, since it does mean that the function was completely
cold. Otherwise we end up treating these functions conservatively
in isFunctionEntryCold() and isColdBB().

Instead, for SamplePGO use -1 when there are no samples, and change
getEntryCount to return None when the value is -1.

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

1586 ↗

(On Diff #127184)

Add a comment here and reference the treatment of -1 in getEntryCount()

[tejohnson]

Address comment

[davidxl]

lgtm

[danielcdh]

LGTM, I'm curious what is the code size impact on PGO binaries (I'd expect it to reduce code size significantly)

llvm/trunk/lib/IR/Function.cpp

 
 Optional<uint64_t> Function::getEntryCount() const {
   MDNode *MD = getMetadata(LLVMContext::MD_prof);
   if (MD && MD->getOperand(0))
     if (MDString *MDS = dyn_cast<MDString>(MD->getOperand(0)))
       if (MDS->getString().equals("function_entry_count")) {
         ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(1));
         uint64_t Count = CI->getValue().getZExtValue();
-        if (Count == 0)
+        // A value of -1 is used for SamplePGO when there were no samples.
+        // Treat this the same as unknown.
+        if (Count == (uint64_t)-1)
           return None;
         return Count;
       }
   return None;
 }
 
 DenseSet<GlobalValue::GUID> Function::getImportGUIDs() const {
   DenseSet<GlobalValue::GUID> R;

llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp

 
 bool SampleProfileLoaderLegacyPass::runOnModule(Module &M) {
   ACT = &getAnalysis<AssumptionCacheTracker>();
   TTIWP = &getAnalysis<TargetTransformInfoWrapperPass>();
   return SampleLoader.runOnModule(M, nullptr);
 }
 
 bool SampleProfileLoader::runOnFunction(Function &F, ModuleAnalysisManager *AM) {
-  F.setEntryCount(0);
+  // Initialize the entry count to -1, which will be treated conservatively
+  // by getEntryCount as the same as unknown (None). If we have samples this
+  // will be overwritten in emitAnnotations.
+  F.setEntryCount(-1);
   std::unique_ptr<OptimizationRemarkEmitter> OwnedORE;
   if (AM) {
     auto &FAM =
         AM->getResult<FunctionAnalysisManagerModuleProxy>(*F.getParent())
             .getManager();
     ORE = &FAM.getResult<OptimizationRemarkEmitterAnalysis>(F);
   } else {
     OwnedORE = make_unique<OptimizationRemarkEmitter>(&F);

llvm/trunk/test/Transforms/SampleProfile/entry_counts.ll

 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/entry_counts.prof -S | FileCheck %s
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/entry_counts.prof -S | FileCheck %s
 
 ; According to the profile, function empty() was called 13,293 times.
 ; CHECK: {{.*}} = !{!"function_entry_count", i64 13294}
 
 define void @empty() !dbg !4 {
 entry:
   ret void, !dbg !9
 }
 
-; This function does not have profile, check if function_entry_count is 0
-; CHECK: {{.*}} = !{!"function_entry_count", i64 0}
+; This function does not have profile, check if function_entry_count is -1
+; CHECK: {{.*}} = !{!"function_entry_count", i64 -1}
 define void @no_profile() {
 entry:
   ret void
 }
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!6, !7}
 !llvm.ident = !{!8}