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

[Vedant Kumar via llvm-commits]

> On Oct 18, 2016, at 11:34 AM, Rong Xu <xur at google.com> wrote:
> 
> 
> 
> 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.

Sorry, my mistake.


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

I still prefer this if only for the API consistency.

thanks,
vedant

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