[PATCH] Introduce the InstrProfReader interface and a text reader

[Eric Christopher]

Agreed, thanks.

-eric

On Fri, Mar 21, 2014 at 10:11 AM, Bob Wilson <bob.wilson at apple.com> wrote:
> 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits