[SampleFDO] Report error when reading a bad/incompatible profile instead of turning off profile use silently.

[wmi]

Summary

Currently sample loader pass turns off SampleFDO optimization silently when it sees error in reading the profile. This behavior will defeat the tests which could have caught those bad/incompatible profile problems and let the problem slip to release or production. This patch change the behavior and report an error when reading a bad profile.

[davidxl]

Should it be warning or error?

[wmi]

In D95269#2516884, @davidxl wrote:

Should it be warning or error?

A build command has samplefdo flag in place, but it cannot perform the samplefdo optimization because the input profile cannot be read properly. That is an error case which can be fixed. So I think it should be an error. From my understanding, a warning should be used in the case that there is something concerning but likely there is nothing to be fixed, like profile mismatch in instrument FDO.

Just above the error reporting I added, there has already been another error handling dealing the case that a profile cannot be opened. I think these two cases are similar.

[davidxl]

lgtm

[wenlei]

LGTM. Thanks for tightening this up. though I think warning does give others more flexibility in how/when to respond to this - in the end, this is not a build break, but a performance issue.

[hoy]

LGTM, thanks.

llvm/lib/Transforms/IPO/SampleProfile.cpp

protected:
   FunctionSamples *Samples = nullptr;
 
   /// Name of the profile file to load.
   std::string Filename;
 
   /// Name of the profile remapping file to load.
   std::string RemappingFilename;
 
-  /// Flag indicating whether the profile input loaded successfully.
-  bool ProfileIsValid = false;
-
   /// Flag indicating if the pass is invoked in ThinLTO compile phase.
   ///
   /// In this phase, in annotation, we should not promote indirect calls.
   /// Instead, we will mark GUIDs that needs to be annotated to the function.
   bool IsThinLTOPreLink;
 
   /// Profile Summary Info computed from sample profile.
   ProfileSummaryInfo *PSI = nullptr;
auto ReaderOrErr =
       SampleProfileReader::create(Filename, Ctx, RemappingFilename);
   if (std::error_code EC = ReaderOrErr.getError()) {
     std::string Msg = "Could not open profile: " + EC.message();
     Ctx.diagnose(DiagnosticInfoSampleProfile(Filename, Msg));
     return false;
   }
   Reader = std::move(ReaderOrErr.get());
   Reader->collectFuncsFrom(M);
-  ProfileIsValid = (Reader->read() == sampleprof_error::success);
+  if (std::error_code EC = Reader->read()) {
+    std::string Msg = "profile reading failed: " + EC.message();
+    Ctx.diagnose(DiagnosticInfoSampleProfile(Filename, Msg));
+    return false;
+  }
+
   PSL = Reader->getProfileSymbolList();
 
   // While profile-sample-accurate is on, ignore symbol list.
   ProfAccForSymsInList =
       ProfileAccurateForSymsInList && PSL && !ProfileSampleAccurate;
   if (ProfAccForSymsInList) {
     NamesInProfile.clear();
     if (auto NameTable = Reader->getNameTable())

 }
 
 ModulePass *llvm::createSampleProfileLoaderPass(StringRef Name) {
   return new SampleProfileLoaderLegacyPass(Name);
 }
 
 bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM,
                                       ProfileSummaryInfo *_PSI, CallGraph *CG) {
-  if (!ProfileIsValid)
-    return false;
   GUIDToFuncNameMapper Mapper(M, *Reader, GUIDToFuncNameMap);
 
   PSI = _PSI;
   if (M.getProfileSummary(/* IsCS */ false) == nullptr) {
     M.setProfileSummary(Reader->getSummary().getMD(M.getContext()),
                         ProfileSummary::PSK_Sample);
     PSI->refresh();
   }

llvm/test/Transforms/SampleProfile/profile-format.ll

 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/inline.prof -S | FileCheck %s
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/inline.prof -S | FileCheck %s
 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/inline.compactbinary.afdo -S | FileCheck %s
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/inline.compactbinary.afdo -S | FileCheck %s
 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/inline.extbinary.afdo -S | FileCheck %s
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/inline.extbinary.afdo -S | FileCheck %s
 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/inline.md5extbinary.afdo -S | FileCheck %s
 ; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/inline.md5extbinary.afdo -S | FileCheck %s
+; RUN: not opt < %s -sample-profile -sample-profile-file=%S/Inputs/bad.extbinary.afdo -S 2>&1 | FileCheck %s -check-prefix=BAD-PROFILE
+; RUN: not opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/bad.extbinary.afdo -S 2>&1 | FileCheck %s -check-prefix=BAD-PROFILE
 
 ; Original C++ test case
 ;
 ; #include <stdio.h>
 ;
 ; int sum(int x, int y) {
 ;   return x + y;
 ; }

 ; CHECK: br i1 %cmp1, label %if.then, label %if.else{{.*}} !prof ![[IDX2:[0-9]*]]
 ; CHECK: call i32 (i8*, ...) @printf{{.*}} !prof ![[IDX3:[0-9]*]]
 ; CHECK: = !{!"TotalCount", i64 26781}
 ; CHECK: = !{!"MaxCount", i64 5553}
 ; CHECK: ![[IDX1]] = !{!"branch_weights", i32 5392, i32 163}
 ; CHECK: ![[IDX2]] = !{!"branch_weights", i32 5280, i32 113}
 ; CHECK: ![[IDX3]] = !{!"branch_weights", i32 1}
 
+; Check sample-profile phase will report error when it is reading a bad profile.
+; BAD-PROFILE: error: {{.*}}bad.extbinary.afdo: profile reading failed: Malformed sample profile data
+
 ; Function Attrs: nounwind uwtable
 define i32 @_Z3sumii(i32 %x, i32 %y) #0 !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