[PGO] fix bogus warning for merging empty llvm profile file

[xur]

Summary

Profile runtime can generate an empty profile (when there is no function in the shared library). This empty profile is treated as a text format profile. A test format profile without the flag of "#IR" is thought to be a clang generated profile. So in llvm profile merging, we will get a bogus warning of "Merge IR generated profile with Clang generated profile."

The fix here is to skip the empty profile (when the buffer size is 0) by returning nullptr in InstrProfReader::create().

This patch also fixes a use-after-move instance in loadInput() in llvm-profdata.cpp.

[davidxl]

A test case is needed.

tools/llvm-profdata/llvm-profdata.cpp

159 ↗

(On Diff #74873)

unrelated change?

[xur]

tools/llvm-profdata/llvm-profdata.cpp

159 ↗

(On Diff #74873)

this is for the use-after-move fix I mentioned in the description.
You want to split to another patch?

[vsk]

tools/llvm-profdata/llvm-profdata.cpp

159 ↗

(On Diff #74873)

I think it'd be better to land this change separately.

[xur]

Integrated the review comments from David and Vedant:
(1) add a test,
(2) split the use-after-move fix to another patch.

Thanks!

-Rong

[davidxl]

test/tools/llvm-profdata/merge_empty_profile.test

3 ↗

(On Diff #75028)

there is no need to check in an empty file. Just 'touch %t_empty.proftext' or 'echo -n "" > %t_empty.proftext"

[vsk]

Thanks for splitting the patch up. Please upload diffs with more context in the future (e.g git diff -U1000).

include/llvm/ProfileData/InstrProfReader.h

108 ↗

(On Diff #75028)

This breaks CoverageMapping::load(), and creates an inconsistency between the API's for InstrProfReader::create and IndexedInstrProfReader::create (one can successfully return nullptr, the other can't).

I'd prefer for this functionality to be implemented via a new instrprof_error. Clients can opt into handling this as a 'soft'/recoverable error, or fail with an appropriate diagnostic packaged up by the Error.

tools/llvm-profdata/llvm-profdata.cpp

149 ↗

(On Diff #75028)

In llvm-profdata's loadInput, you could write:

if ((Error E = ReaderOrErr.takeError())) {
  if (E != instrprof_error::empty_profile)
    WC->Err = std::move(E);
  return;
}

[xur]

Followed vsk's suggestion to use a new error code for empty file case.
Note that this does not apply to indexed profile where we don't expected empty file (in which case, we will report a format error).

Also changed to use 'touch' rather than the empty profile in Inputs (suggested by david).

-Rong

[vsk]

Thanks! lgtm with a nit.

lib/ProfileData/InstrProf.cpp

74 ↗

(On Diff #75233)

Could you add a space between raw and profile?

llvm/trunk/include/llvm/ProfileData/InstrProf.h

enum class instrprof_error {
   truncated,
   malformed,
   unknown_function,
   hash_mismatch,
   count_mismatch,
   counter_overflow,
   value_site_count_mismatch,
   compress_failed,
-  uncompress_failed
+  uncompress_failed,
+  empty_raw_profile
 };
 
 inline std::error_code make_error_code(instrprof_error E) {
   return std::error_code(static_cast<int>(E), instrprof_category());
 }
 
 class InstrProfError : public ErrorInfo<InstrProfError> {
 public:

llvm/trunk/lib/ProfileData/InstrProf.cpp

std::string getInstrProfErrString(instrprof_error Err) {
   case instrprof_error::counter_overflow:
     return "Counter overflow";
   case instrprof_error::value_site_count_mismatch:
     return "Function value site count change detected (counter mismatch)";
   case instrprof_error::compress_failed:
     return "Failed to compress data (zlib)";
   case instrprof_error::uncompress_failed:
     return "Failed to uncompress data (zlib)";
+  case instrprof_error::empty_raw_profile:
+    return "Empty raw profile file";
   }
   llvm_unreachable("A value of instrprof_error has no message.");
 }
 
 // FIXME: This class is only here to support the transition to llvm::Error. It
 // will be removed once this transition is complete. Clients should prefer to
 // deal with the Error value directly, rather than converting to error_code.
 class InstrProfErrorCategoryType : public std::error_category {

llvm/trunk/lib/ProfileData/InstrProfReader.cpp

 }
 
 Expected<std::unique_ptr<InstrProfReader>>
 InstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer) {
   // Sanity check the buffer.
   if (Buffer->getBufferSize() > std::numeric_limits<unsigned>::max())
     return make_error<InstrProfError>(instrprof_error::too_large);
 
+  if (Buffer->getBufferSize() == 0)
+    return make_error<InstrProfError>(instrprof_error::empty_raw_profile);
+
   std::unique_ptr<InstrProfReader> Result;
   // Create the reader.
   if (IndexedInstrProfReader::hasFormat(*Buffer))
     Result.reset(new IndexedInstrProfReader(std::move(Buffer)));
   else if (RawInstrProfReader64::hasFormat(*Buffer))
     Result.reset(new RawInstrProfReader64(std::move(Buffer)));
   else if (RawInstrProfReader32::hasFormat(*Buffer))
     Result.reset(new RawInstrProfReader32(std::move(Buffer)));

llvm/trunk/test/tools/llvm-profdata/Inputs/IR_profile.proftext

+:ir
+main
+# Func Hash:
[phone]
+# Num Counters:
+1
+# Counter Values:
+1
+

llvm/trunk/test/tools/llvm-profdata/Inputs/clang_profile.proftext

+main
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+1
+

llvm/trunk/test/tools/llvm-profdata/merge_empty_profile.test

+# Tests for merge of empty profile files.
+
+RUN: touch %t_empty.proftext
+RUN: llvm-profdata merge -text -o %t_clang.proftext %t_empty.proftext %p/Inputs/clang_profile.proftext
+RUN: FileCheck --input-file=%t_clang.proftext %s -check-prefix=CLANG_PROF_TEXT
+CLANG_PROF_TEXT: main
+CLANG_PROF_TEXT: 0
+CLANG_PROF_TEXT: 1
+CLANG_PROF_TEXT: 1
+
+RUN: llvm-profdata merge -text -o %t_ir.proftext %t_empty.proftext %p/Inputs/IR_profile.proftext
+RUN: FileCheck --input-file=%t_ir.proftext %s -check-prefix=IR_PROF_TEXT
+IR_PROF_TEXT: :ir
+IR_PROF_TEXT: main
+IR_PROF_TEXT: 0
+IR_PROF_TEXT: 1
+IR_PROF_TEXT: 1

llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp

static void loadInput(const WeightedFile &Input, WriterContext *WC) {
 
   // If there's a pending hard error, don't do more work.
   if (WC->Err)
     return;
 
   WC->ErrWhence = Input.Filename;
 
   auto ReaderOrErr = InstrProfReader::create(Input.Filename);
-  if ((WC->Err = ReaderOrErr.takeError()))
+  if (Error E = ReaderOrErr.takeError()) {
+    // Skip the empty profiles by returning sliently.
+    instrprof_error IPE = InstrProfError::take(std::move(E));
+    if (IPE != instrprof_error::empty_raw_profile)
+      WC->Err = make_error<InstrProfError>(IPE);
     return;
+  }
 
   auto Reader = std::move(ReaderOrErr.get());
   bool IsIRProfile = Reader->isIRLevelProfile();
   if (WC->Writer.setIsIRLevelProfile(IsIRProfile)) {
     WC->Err = make_error<StringError>(
         "Merge IR generated profile with Clang generated profile.",
         std::error_code());
     return;