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

[Rong Xu via llvm-commits]

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

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

Sure. I'll update the patch with this approach.

thanks!


>
> 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
> >
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161018/a0d24c98/attachment.html>