[SampleFDO] Create a separate flag ProfileAccurateForProfSymList for handling profile symbol list

[wmi]

Summary

Currently many existing users using profile-sample-accurate want to reduce code size as much as possible. Their use cases are different from the scenario profile symbol list tries to handle -- the major motivation of adding profile symbol list is to get the major memory/code size saving without introduce performance regression. So to keep the behavior of profile-sample-accurate unchanged, we think decoupling these two things and use a new flag to control the handling of profile symbol list is better.

Besides the reason above, another benefit of adding a separate flag is we can flip on ProfileAccurateForProfSymList by default, but that flip will only affect targets using profile with symbol list. Targets won't see any impact as long as their profiles don't contain symbol list. It is possible to get the memory saving by just adding profile symbol list in the profile so it is easier for deployment.

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

134 ↗

(On Diff #221823)

profile-accurate-for-symbols-in-list ?

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

134 ↗

(On Diff #221823)

It should be true by default.

136 ↗

(On Diff #221823)

are --> to be.

[wmi]

Address David's comment.

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

1674 ↗

(On Diff #221856)

check PSL too?

1782 ↗

(On Diff #221856)

If symbol list is available, it will override the decision of -fprofile-sample-accurate. Is this the intended behavior?

[wmi]

lib/Transforms/IPO/SampleProfile.cpp

1674 ↗

(On Diff #221856)

Fixed.

1782 ↗

(On Diff #221856)

Good point. I feel it is safer to keep the profile-sample-accurate behavior the same especially if large scale rollout of PSL is needed, so I will change it.

[wmi]

Address David's comment: profile-accurate-for-symsinlist will be overriden by profile-sample-accurate if both flags present. Add test for it.

[wmi]

Update the flag description of profile-accurate-for-symsinlist.

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

502 ↗

(On Diff #222202)

Add an assert here that profile-sample-accurate is not on?

1682 ↗

(On Diff #222202)

Add a comment that while profile-sample-accurate is on, ignore symbol list.

1786 ↗

(On Diff #222202)

Improve the comment to be: " profile-sample-accurate is a user assertion which has a higher precedence than symbol list. When profile-sample-accurate is on, ignore symbol list.

[wmi]

lib/Transforms/IPO/SampleProfile.cpp

502 ↗

(On Diff #222202)

I add an assertion in the caller of callsiteIsHot, so we can check the case that profile-sample-accurate is set in function attribute.

[wmi]

Address David's comment.

[davidxl]

lgtm

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

cl::desc("Use this option to turn off/on warnings about function with "
              "samples but without debug information to use those samples. "));
 
 static cl::opt<bool> ProfileSampleAccurate(
     "profile-sample-accurate", cl::Hidden, cl::init(false),
     cl::desc("If the sample profile is accurate, we will mark all un-sampled "
              "callsite and function as having 0 samples. Otherwise, treat "
              "un-sampled callsites and functions conservatively as unknown. "));
 
+static cl::opt<bool> ProfileAccurateForSymsInList(
+    "profile-accurate-for-symsinlist", cl::Hidden, cl::ZeroOrMore,
+    cl::init(true),
+    cl::desc("For symbols in profile symbol list, regard their profiles to "
+             "be accurate. It may be overriden by profile-sample-accurate. "));
+
 namespace {
 
 using BlockWeightMap = DenseMap<const BasicBlock *, uint64_t>;
 using EquivalenceClassMap = DenseMap<const BasicBlock *, const BasicBlock *>;
 using Edge = std::pair<const BasicBlock *, const BasicBlock *>;
 using EdgeWeightMap = DenseMap<Edge, uint64_t>;
 using BlockEdgeMap =
     DenseMap<const BasicBlock *, SmallVector<const BasicBlock *, 8>>;
protected:
   // GUIDToFuncNameMap saves the mapping from GUID to the symbol name, for
   // all the function symbols defined or declared in current module.
   DenseMap<uint64_t, StringRef> GUIDToFuncNameMap;
 
   // All the Names used in FunctionSamples including outline function
   // names, inline instance names and call target names.
   StringSet<> NamesInProfile;
 
-  // Showing whether ProfileSampleAccurate is enabled for current function.
-  bool ProfSampleAccEnabled = false;
+  // For symbol in profile symbol list, whether to regard their profiles
+  // to be accurate. It is mainly decided by existance of profile symbol
+  // list and -profile-accurate-for-symsinlist flag, but it can be
+  // overriden by -profile-sample-accurate or profile-sample-accurate
+  // attribute.
+  bool ProfAccForSymsInList;
 };
 
 class SampleProfileLoaderLegacyPass : public ModulePass {
 public:
   // Class identification, replacement for typeinfo
   static char ID;
 
   SampleProfileLoaderLegacyPass(StringRef Name = SampleProfileFile,

 /// the original inline decision was "good" (i.e., the callsite is executed
 /// frequently), then we will recreate the inline decision and apply the
 /// profile from the inlined callsite.
 ///
 /// To decide whether an inlined callsite is hot, we compare the callsite
 /// sample count with the hot cutoff computed by ProfileSummaryInfo, it is
 /// regarded as hot if the count is above the cutoff value.
 ///
-/// When profile-sample-accurate is enabled, functions without profile will
+/// When ProfileAccurateForSymsInList is enabled and profile symbol list
+/// is present, functions in the profile symbol list but without profile will
 /// be regarded as cold and much less inlining will happen in CGSCC inlining
 /// pass, so we tend to lower the hot criteria here to allow more early
 /// inlining to happen for warm callsites and it is helpful for performance.
 bool SampleProfileLoader::callsiteIsHot(const FunctionSamples *CallsiteFS,
                                         ProfileSummaryInfo *PSI) {
   if (!CallsiteFS)
     return false; // The callsite was not inlined in the original binary.
 
   assert(PSI && "PSI is expected to be non null");
   uint64_t CallsiteTotalSamples = CallsiteFS->getTotalSamples();
-  if (ProfSampleAccEnabled)
+  if (ProfAccForSymsInList)
     return !PSI->isColdCount(CallsiteTotalSamples);
   else
     return PSI->isHotCount(CallsiteTotalSamples);
 }
 
 /// Mark as used the sample record for the given function samples at
 /// (LineOffset, Discriminator).
 ///

 /// \param InlinedGUIDs a set to be updated to include all GUIDs that are
 ///     inlined in the profiled binary.
 ///
 /// \returns True if there is any inline happened.
 bool SampleProfileLoader::inlineHotFunctions(
     Function &F, DenseSet<GlobalValue::GUID> &InlinedGUIDs) {
   DenseSet<Instruction *> PromotedInsns;
 
+  // ProfAccForSymsInList is used in callsiteIsHot. The assertion makes sure
+  // Profile symbol list is ignored when profile-sample-accurate is on.
+  assert((!ProfAccForSymsInList ||
+          (!ProfileSampleAccurate &&
+           !F.hasFnAttribute("profile-sample-accurate"))) &&
+         "ProfAccForSymsInList should be false when profile-sample-accurate "
+         "is enabled");
+
   DenseMap<Instruction *, const FunctionSamples *> localNotInlinedCallSites;
   bool Changed = false;
   while (true) {
     bool LocalChanged = false;
     SmallVector<Instruction *, 10> CIS;
     for (auto &BB : F) {
       bool Hot = false;
       SmallVector<Instruction *, 10> Candidates;
if (std::error_code EC = ReaderOrErr.getError()) {
     Ctx.diagnose(DiagnosticInfoSampleProfile(Filename, Msg));
     return false;
   }
   Reader = std::move(ReaderOrErr.get());
   Reader->collectFuncsToUse(M);
   ProfileIsValid = (Reader->read() == sampleprof_error::success);
   PSL = Reader->getProfileSymbolList();
 
-  if (ProfileSampleAccurate) {
+  // While profile-sample-accurate is on, ignore symbol list.
+  ProfAccForSymsInList =
+      ProfileAccurateForSymsInList && PSL && !ProfileSampleAccurate;
+  if (ProfAccForSymsInList) {
     NamesInProfile.clear();
     if (auto NameTable = Reader->getNameTable())
       NamesInProfile.insert(NameTable->begin(), NameTable->end());
   }
 
   if (!RemappingFilename.empty()) {
     // Apply profile remappings to the loaded profile data if requested.
     // For now, we only support remapping symbols encoded using the Itanium
bool SampleProfileLoader::runOnFunction(Function &F, ModuleAnalysisManager *AM) {
 
   DILocation2SampleMap.clear();
   // By default the entry count is initialized to -1, which will be treated
   // conservatively by getEntryCount as the same as unknown (None). This is
   // to avoid newly added code to be treated as cold. If we have samples
   // this will be overwritten in emitAnnotations.
   uint64_t initialEntryCount = -1;
 
-  ProfSampleAccEnabled =
-      ProfileSampleAccurate || F.hasFnAttribute("profile-sample-accurate");
-  if (ProfSampleAccEnabled) {
-    // PSL -- profile symbol list include all the symbols in sampled binary.
-    // It is used to prevent new functions to be treated as cold.
-    if (PSL) {
-      // Profile symbol list is available, initialize the entry count to 0
-      // only for functions in the list.
-      if (PSL->contains(F.getName()))
-        initialEntryCount = 0;
+  ProfAccForSymsInList = ProfileAccurateForSymsInList && PSL;
+  if (ProfileSampleAccurate || F.hasFnAttribute("profile-sample-accurate")) {
+    // initialize all the function entry counts to 0. It means all the
+    // functions without profile will be regarded as cold.
+    initialEntryCount = 0;
+    // profile-sample-accurate is a user assertion which has a higher precedence
+    // than symbol list. When profile-sample-accurate is on, ignore symbol list.
+    ProfAccForSymsInList = false;
+  }
+
+  // If ProfileAccurateForSymsInList is enabled, PSL is used to treat
+  // old functions without samples being cold, without having to worry
+  // about new and hot functions being mistakenly treated as cold.
+  if (ProfAccForSymsInList) {
+    // Initialize the entry count to 0 for functions in the list.
 
-      // When ProfileSampleAccurate is true, function without sample will be
-      // regarded as cold. To minimize the potential negative performance
-      // impact it could have, we want to be a little conservative here
-      // saying if a function shows up in the profile, no matter as outline
-      // function, inline instance or call targets, treat the function as not
-      // being cold. This will handle the cases such as most callsites of a
-      // function are inlined in sampled binary but not inlined in current
-      // build (because of source code drift, imprecise debug information, or
-      // the callsites are all cold individually but not cold
-      // accumulatively...), so the outline function showing up as cold in
-      // sampled binary will actually not be cold after current build.
-      StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
-      if (NamesInProfile.count(CanonName))
-        initialEntryCount = -1;
-    } else {
-      // If there is no profile symbol list available, initialize all the
-      // function entry counts to 0. It means all the functions without
-      // profile will be regarded as cold.
-      initialEntryCount = 0;
-    }
+    // Function in the symbol list but without sample will be regarded as
+    // cold. To minimize the potential negative performance impact it could
+    // have, we want to be a little conservative here saying if a function
+    // shows up in the profile, no matter as outline function, inline instance
+    // or call targets, treat the function as not being cold. This will handle
+    // the cases such as most callsites of a function are inlined in sampled
+    // binary but not inlined in current build (because of source code drift,
+    // imprecise debug information, or the callsites are all cold individually
+    // but not cold accumulatively...), so the outline function showing up as
+    // cold in sampled binary will actually not be cold after current build.
   }
 
   F.setEntryCount(ProfileCount(initialEntryCount, Function::PCT_Real));
   std::unique_ptr<OptimizationRemarkEmitter> OwnedORE;
   if (AM) {
     auto &FAM =
         AM->getResult<FunctionAnalysisManagerModuleProxy>(*F.getParent())
             .getManager();

llvm/trunk/test/Transforms/SampleProfile/compressed-profile-symbol-list.ll

 ; REQUIRES: zlib
 ; Append inline.prof with profile symbol list and save it after compression.
 ; RUN: llvm-profdata merge --sample --prof-sym-list=%S/Inputs/profile-symbol-list.text --compress-prof-sym-list=true --extbinary %S/Inputs/inline.prof --output=%t.profdata
-; RUN: opt < %S/Inputs/profile-symbol-list.ll -sample-profile -profile-sample-accurate -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll
-; RUN: opt < %S/Inputs/profile-symbol-list.ll -passes=sample-profile -profile-sample-accurate -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll
+; RUN: opt < %S/Inputs/profile-symbol-list.ll -sample-profile -profile-accurate-for-symsinlist -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll
+; RUN: opt < %S/Inputs/profile-symbol-list.ll -passes=sample-profile -profile-accurate-for-symsinlist -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll

llvm/trunk/test/Transforms/SampleProfile/profile-sample-accurate.ll

-; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -S | FileCheck %s
-; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -S | FileCheck %s
+; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -S | FileCheck %s --check-prefix=CALL_SUM_IS_WARM
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=900000 -profile-sample-accurate -S | FileCheck %s --check-prefix=CALL_SUM_IS_HOT
+; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -S | FileCheck %s --check-prefix=CALL_SUM_IS_WARM
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=900000 -profile-sample-accurate -S | FileCheck %s --check-prefix=CALL_SUM_IS_HOT
+
 ; RUN: llvm-profdata merge -sample -extbinary -prof-sym-list=%S/Inputs/profile-symbol-list.text %S/Inputs/profsampleacc.extbinary.afdo -o %t.symlist.afdo
-; RUN: opt < %s -sample-profile -sample-profile-file=%t.symlist.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -S | FileCheck %s --check-prefix=PROFSYMLIST
-; RUN: opt < %s -passes=sample-profile -sample-profile-file=%t.symlist.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -S | FileCheck %s --check-prefix=PROFSYMLIST
+; RUN: opt < %s -sample-profile -sample-profile-file=%t.symlist.afdo -profile-summary-cutoff-hot=600000 -profile-accurate-for-symsinlist -S | FileCheck %s --check-prefix=PROFSYMLIST
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%t.symlist.afdo -profile-summary-cutoff-hot=600000 -profile-accurate-for-symsinlist -S | FileCheck %s --check-prefix=PROFSYMLIST
+;
+; If -profile-accurate-for-symsinlist and -profile-sample-accurate both present,
+; -profile-sample-accurate will override -profile-accurate-for-symsinlist.
+; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -profile-accurate-for-symsinlist -S | FileCheck %s --check-prefix=CALL_SUM_IS_WARM
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=900000 -profile-sample-accurate -profile-accurate-for-symsinlist -S | FileCheck %s --check-prefix=CALL_SUM_IS_HOT
+; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=600000 -profile-sample-accurate -profile-accurate-for-symsinlist -S | FileCheck %s --check-prefix=CALL_SUM_IS_WARM
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/profsampleacc.extbinary.afdo -profile-summary-cutoff-hot=900000 -profile-sample-accurate -profile-accurate-for-symsinlist -S | FileCheck %s --check-prefix=CALL_SUM_IS_HOT
 ;
 ; Original C++ test case
 ;
 ; #include <stdio.h>
 ;
 ; int sum(int x, int y) {
 ;   return x + y;
 ; }
 ;
 ; int main() {
 ;   int s, i = 0;
 ;   while (i++ < 20000 * 20000)
 ;     if (i != 100) s = sum(i, s); else s = 30;
 ;   printf("sum is %d\n", s);
 ;   return 0;
 ; }
 ;
 @.str = private unnamed_addr constant [11 x i8] c"sum is %d\0A\00", align 1
 
-; Check _Z3sumii's function entry count will be intialized to 0 if no profile
-; symbol list is available.
-; If symbol list is available, _Z3sumii's function entry count will be
-; initialized to -1 if it shows up in the profile.
+; Check _Z3sumii's function entry count will be 0 when
+; profile-sample-accurate is enabled.
+; CALL_SUM_IS_HOT: define i32 @_Z3sumii{{.*}}!prof ![[ZERO_ID:[0-9]+]]
 ;
-; CHECK: define i32 @_Z3sumii{{.*}}!prof ![[ZERO_ID:[0-9]+]]
+; Check _Z3sumii's function entry count will be nonzero when
+; profile-sample-accurate is enabled because the callsite is warm and not
+; inlined so its function entry count is adjusted to nonzero.
+; CALL_SUM_IS_WARM: define i32 @_Z3sumii{{.*}}!prof ![[NONZERO_ID:[0-9]+]]
+;
+; Check _Z3sumii's function entry count will be initialized to -1 when
+; profile-accurate-for-profsymlist is enabled and _Z3sumii exists in the
+; profile symbol list because it also shows up in the profile as inline
+; instance.
 ; PROFSYMLIST: define i32 @_Z3sumii{{.*}}!prof ![[UNKNOWN_ID:[0-9]+]]
+;
 ; Function Attrs: nounwind uwtable
 define i32 @_Z3sumii(i32 %x, i32 %y) !dbg !4 {
 entry:
   %x.addr = alloca i32, align 4
   %y.addr = alloca i32, align 4
   store i32 %x, i32* %x.addr, align 4
   store i32 %y, i32* %y.addr, align 4
   %0 = load i32, i32* %x.addr, align 4, !dbg !11
while.cond:                                       ; preds = %if.end, %entry
   br i1 %cmp, label %while.body, label %while.end, !dbg !14
 
 while.body:                                       ; preds = %while.cond
   %1 = load i32, i32* %i, align 4, !dbg !16
   %cmp1 = icmp ne i32 %1, 100, !dbg !16
   br i1 %cmp1, label %if.then, label %if.else, !dbg !16
 
 ; With the hot cutoff being set to 600000, the inline instance of _Z3sumii
-; in main is neither hot nor cold. Check it will still be inlined when
+; in main is neither hot nor cold. Check it won't be inlined when
 ; profile-sample-accurate is enabled.
-; CHECK: if.then:
-; CHECK-NOT: call i32 @_Z3sumii
-; CHECK: if.else:
+; CALL_SUM_IS_WARM: if.then:
+; CALL_SUM_IS_WARM: call i32 @_Z3sumii
+; CALL_SUM_IS_WARM: if.else:
+;
+; With the hot cutoff being set to 900000, the inline instance of _Z3sumii
+; in main is hot. Check the callsite of _Z3sumii will be inlined when
+; profile-sample-accurate is enabled.
+; CALL_SUM_IS_HOT: if.then:
+; CALL_SUM_IS_HOT-NOT: call i32 @_Z3sumii
+; CALL_SUM_IS_HOT: if.else:
+;
+; Check _Z3sumii will be inlined when profile-accurate-for-profsymlist is
+; enabled
 ; PROFSYMLIST: if.then:
 ; PROFSYMLIST-NOT: call i32 @_Z3sumii
 ; PROFSYMLIST: if.else:
 if.then:                                          ; preds = %while.body
   %2 = load i32, i32* %i, align 4, !dbg !18
   %3 = load i32, i32* %s, align 4, !dbg !18
   %call = call i32 @_Z3sumii(i32 %2, i32 %3), !dbg !18
   store i32 %call, i32* %s, align 4, !dbg !18

 }
 
 declare i32 @printf(i8*, ...) #2
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!8, !9}
 !llvm.ident = !{!10}
 
-; CHECK: ![[ZERO_ID]] = !{!"function_entry_count", i64 0}
+; CALL_SUM_IS_HOT: ![[ZERO_ID]] = !{!"function_entry_count", i64 0}
+; CALL_SUM_IS_WARM: ![[NONZERO_ID]] = !{!"function_entry_count", i64 5179}
 ; PROFSYMLIST: ![[UNKNOWN_ID]] = !{!"function_entry_count", i64 -1}
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, producer: "clang version 3.5 ", isOptimized: false, emissionKind: NoDebug, file: !1, enums: !2, retainedTypes: !2, globals: !2, imports: !2)
 !1 = !DIFile(filename: "calls.cc", directory: ".")
 !2 = !{}
 !4 = distinct !DISubprogram(name: "sum", line: 3, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !0, scopeLine: 3, file: !1, scope: !5, type: !6, retainedNodes: !2)
 !5 = !DIFile(filename: "calls.cc", directory: ".")
 !6 = !DISubroutineType(types: !2)
 !7 = distinct !DISubprogram(name: "main", line: 7, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !0, scopeLine: 7, file: !1, scope: !5, type: !6, retainedNodes: !2)

llvm/trunk/test/Transforms/SampleProfile/uncompressed-profile-symbol-list.ll

 ; Append inline.prof with profile symbol list and save it without compression.
 ; RUN: llvm-profdata merge --sample --prof-sym-list=%S/Inputs/profile-symbol-list.text --compress-prof-sym-list=false --extbinary %S/Inputs/inline.prof --output=%t.profdata
-; RUN: opt < %S/Inputs/profile-symbol-list.ll -sample-profile -profile-sample-accurate -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll
-; RUN: opt < %S/Inputs/profile-symbol-list.ll -passes=sample-profile -profile-sample-accurate -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll
+; RUN: opt < %S/Inputs/profile-symbol-list.ll -sample-profile -profile-accurate-for-symsinlist -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll
+; RUN: opt < %S/Inputs/profile-symbol-list.ll -passes=sample-profile -profile-accurate-for-symsinlist -sample-profile-file=%t.profdata -S | FileCheck %S/Inputs/profile-symbol-list.ll