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

[Rong Xu via llvm-commits]

On Tue, Oct 18, 2016 at 11:14 AM, Vedant Kumar <vsk at apple.com> wrote:

> vsk added a comment.
>
> Thanks for splitting the patch up. Please upload diffs with more context
> in the future (e.g git diff -U1000).
>
> Sorry about this. Will add -U99999 later.


>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProfReader.h:108
> +  /// instrprof file. Nullptr will be returned for an empty profile file.
>    static Expected<std::unique_ptr<InstrProfReader>> create(const Twine
> &Path);
>
> ----------------
> 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 don't see why this breaks CoverageMapping::load() as this API is not
referenced there.
It's only called in llvm-profiledata.cpp.

The difference InstrProfReader::create and IndexedInstrProfReader::create
are intentional as the later will be referenced in CoverageMapping, Clang
PGO and IR PGO etc.


>
> 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.
>

yes. this also works.


>
>
> ================
> Comment at: tools/llvm-profdata/llvm-profdata.cpp:149
> +  if (!Reader)
> +    return;
>    bool IsIRProfile = Reader->isIRLevelProfile();
> ----------------
> 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;
> }
> ```
>
>
> https://reviews.llvm.org/D25687
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161018/72017f8a/attachment.html>