Page MenuHomePhabricator

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

Authored by xur on Oct 17 2016, 11:22 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

xur updated this revision to Diff 74873.Oct 17 2016, 11:22 AM
xur retitled this revision from to [PGO] fix bogus warning for merging empty llvm profile file.
xur updated this object.
xur added a reviewer: davidxl.
xur added a subscriber: llvm-commits.
davidxl edited edge metadata.Oct 17 2016, 11:52 AM

A test case is needed.

tools/llvm-profdata/llvm-profdata.cpp
159 ↗(On Diff #74873)

unrelated change?

xur added inline comments.Oct 17 2016, 12:40 PM
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 added a subscriber: vsk.Oct 17 2016, 12:54 PM
vsk added inline comments.
tools/llvm-profdata/llvm-profdata.cpp
159 ↗(On Diff #74873)

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

xur updated this revision to Diff 75028.Oct 18 2016, 10:26 AM
xur edited edge metadata.

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 added inline comments.Oct 18 2016, 10:36 AM
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 added a comment.Oct 18 2016, 11:14 AM

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 updated this revision to Diff 75233.Oct 19 2016, 2:51 PM

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 accepted this revision.Oct 19 2016, 2:59 PM
vsk added a reviewer: vsk.

Thanks! lgtm with a nit.

lib/ProfileData/InstrProf.cpp
74 ↗(On Diff #75233)

Could you add a space between raw and profile?

This revision is now accepted and ready to land.Oct 19 2016, 2:59 PM
This revision was automatically updated to reflect the committed changes.