This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profdata] Add mode to recover from profile read failures
ClosedPublic

Authored by vsk on Aug 29 2019, 5:27 PM.

Details

Summary

Add a mode in which profile read errors are not immediately treated as
fatal. In this mode, merging makes forward progress and reports failure
only if no inputs can be read.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Aug 29 2019, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 5:27 PM
vsk updated this revision to Diff 218005.Aug 29 2019, 5:33 PM
  • Fix a comment.
vsk updated this revision to Diff 218007.Aug 29 2019, 5:46 PM
  • Don't record a read failure when a soft error is encountered within mergeRecordsFromWriter.

That's a great change, Vedant!! Thank you! //cc @sajjadm @liaoyuke FYI

Might make sense to clarify in the description that this is for merge only, if I understand correctly :)

llvm/tools/llvm-profdata/llvm-profdata.cpp
279 ↗(On Diff #218007)

How bad will it be if we decide to always print the warning here, not just for the first time?

This is a behavior change and I think it should be guarded with an option. The default of the option should be set to 'FailOnAnyWhenMerge' instead of 'FailOnAllWhenMerge'.

vsk planned changes to this revision.Aug 30 2019, 9:06 AM

Need to add an option to preserve the old error reporting behavior. I'll get back to this on Tuesday.

llvm/tools/llvm-profdata/llvm-profdata.cpp
279 ↗(On Diff #218007)

I agree that reporting of soft errors during merging is not as helpful as it should be. We should come up with a better solution. Istm that warning on all soft errors here could be very annoying (e.g. if there are lots of counter overflows).

vsk added a comment.Sep 3 2019, 9:17 AM

@davidxl Are you recommending that the old behavior (fail the merge if any profiles are malformed) should remain the default? I think that's reasonable (it raises the alarm about toolchain bugs), just wanted to double-check.

yes, the old behavior should be the default. It raises the awareness of the problem (and merge errors are usually rare in reality).

vsk added a comment.Sep 3 2019, 9:56 AM

yes, the old behavior should be the default. It raises the awareness of the problem (and merge errors are usually rare in reality).

Right, that's a good point. I wonder if it's worth introducing a separate mode, then, if it's not the recommended way to run the tool in production? Perhaps this should be set aside.

A separate mode is fine. Buffering the errors and reporting them later is also useful (allowing more context info to be emitted).

vsk updated this revision to Diff 218512.Sep 3 2019, 12:12 PM
vsk retitled this revision from [llvm-profdata] Recover from InstrProf read failures to [llvm-profdata] Add mode to recover from profile read failures.
vsk edited the summary of this revision. (Show Details)
  • Allow users to pick the failure mode for profile merging.
davidxl added inline comments.Sep 3 2019, 12:41 PM
llvm/docs/CommandGuide/llvm-profdata.rst
127 ↗(On Diff #218512)

The enums can be named with descriptive strings, but can the command line be simplified? --failure-mode=any|all

vsk updated this revision to Diff 218526.Sep 3 2019, 1:25 PM
  • Shorten failure mode names.
davidxl accepted this revision.Sep 3 2019, 2:15 PM

lgtm

This revision is now accepted and ready to land.Sep 3 2019, 2:15 PM
This revision was automatically updated to reflect the committed changes.