[PATCH] Introduce the InstrProfReader interface and a text reader

[Bob Wilson]

The build changes all look OK to me. Please commit.

On Mar 19, 2014, at 4:45 PM, Justin Bogner <mail at justinbogner.com> wrote:

> "Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>>> This patch introduces the ProfileData library without going into details
>>> about the data format that instrumentation based profiling should use,
>>> instead just sticking to the naive text format we have now.
>>> 
>>> This is designed to make readers for multiple formats simple, so that a
>>> "raw" format for compiler-rt to commit and an efficient format for clang
>>> to consume is easy.
>>> 
>>> Eric, would you mind checking the build system changes? It builds with
>>> both clang and autoconf for me, but I'd appreciate a look over by
>>> someone who has a clue how our build works.
>>> 
>>> <TextInstrProfReader.patch>
>> 
>> LGTM, with a couple of fixups.
>> 
>> But I don’t understand our build system either, so you should wait for
>> someone to check that over.
> 
> Thanks for looking. New patch attached.
> 
>>> +class InstrProfReader {
>>> +  error_code LastError;
>>> +public:
>>> +  InstrProfReader() : LastError(instrprof_error::success) {}
>>> +  virtual ~InstrProfReader() {}
>>> +
>>> +  /// Read a single record.
>>> +  virtual error_code readNextRecord(InstrProfRecord &Record) = 0;
>>> +  /// Iterator over profile data.
>>> +  virtual InstrProfIterator begin() { return InstrProfIterator(this); }
>>> +  virtual InstrProfIterator end() { return InstrProfIterator(); }
>> 
>> Do these need to be virtual?
> 
> Nope!
> 
>>> +
>>> +    for (size_t II = 0, EE = I1->Counts.size(); II < EE; ++II) {
>> 
>> Ranged-based for here?
>> 
>>> +      uint64_t Sum = I1->Counts[II] + I2->Counts[II];
>>> +      if (Sum < I1->Counts[II])
>>> +        exitWithError("Counter overflow for " + I1->Name);
>>> +      Output << Sum << "\n";
>>>     }
>>> +    Output << "\n";
> 
> This iterates over two vectors, so not unless we have a zip() adapter.
> 
>>> +  if (Reader1->hasError())
>>> +    exitWithError(Reader1->getError().message(), Filename1);
>>> +  if (Reader2->hasError())
>>> +    exitWithError(Reader2->getError().message(), Filename1);
>> 
>> Should this be Filename2?
> 
> Yes.
> 
>>> +  if (!Reader1->isEOF() || !Reader2->isEOF())
>>> +    exitWithError("Number of instrumented functions differ.”);
>> 
>> If we’re assuming for now that the first file is canonical, I think
>> you should give Filename2 as the source of the error.
> 
> I prefer it this way. The errors in the loop have no filename, and this
> is the same type of problem.
> 
> <TextInstrProfReader-2.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits