[PATCH] Port Linux Perf converter to LLVM

[Diego Novillo]

On Wed, Aug 13, 2014 at 7:07 PM, Justin Bogner <mail at justinbogner.com> wrote:

> Some tests would be nice.

Added.

> I can't help but think that it'd be nice if the API that other parts of
> LLVM will use follows the LLVM style - ie, the classes in the
> SampleProfileConverter/Reader headers. I'll make some specific nitpicks
> about that below.

Yeah, that's the main thing I want to iterate over. The current API is
using a few local conventions. Another thing that concerns me a bit,
is that the API is probably too tied to Perf. Other sample profilers
will likely need different/more functionality. But I prefer to
add/modify that as needed.

>> +class SampleProfileConverter {
>> + public:
>> +  explicit SampleProfileConverter(const string &binary) : sample_reader_(NULL),
>> +                                                  binary_(binary) {}
>
> This indentation's a bit odd, and I'd prefer nullptr here.

Done.

>> +
>> +  ~SampleProfileConverter() {
>> +    delete sample_reader_;
>> +  }
>
> sample_reader_ could be a unique_ptr to make ownership explicit and save
> writing this destructor, though since it's entirely private it doesn't
> really matter.

Done.

>> +
>> +  // Creates SamplePGO profile, returns true if success, false otherwise.
>
> Please add an extra slash (///) so that doxygen picks these up.

Done.

>> +  bool CreateProfile(const string &input_profile_name,
>> +                     const string &profiler,
>> +                     const string &output_profile_name);
>
> Lower case names for these functions would make using this API more
> LLVM-y.

Done.

>
>> +
>> +  // Reads samples from the input profile.
>> +  bool ReadSample(const string &input_profile_name,
>> +                  const string &profiler);
>> +
>> +  // Creates output profile after reading from the input profile.
>> +  bool CreateProfileFromSample(const string &output_profile_name);
>> +
>> +  // Returns total number of samples collected.
>> +  uint64_t TotalSamples();
>> +
>> +  // Returns the SampleReader pointer.
>> +  const samplepgo::SampleReader &sample_reader() {
>> +    return *sample_reader_;
>> +  }
>
> A name like getSampleReader would be more consistent with other LLVM APIs.

Done.

>> +  // Returns the sample count for a given instruction.
>> +  uint64_t GetSampleCountOrZero(uint64_t addr) const;
>
> See above comments on naming and doxygen comments :)

Done.

>> +  samplepgo::SampleProfileConverter converter(InputBinary);
>> +  if (converter.CreateProfile(InputProfile, ProfileType, OutputProfile))
>> +    return 0;
>> +  else
>> +    return -1;
>
> This should be called "Converter", and I find this easier to read like so:
>
>   samplepgo::SampleProfileConverter Converter(InputBinary);
>   if (!Converter.CreateProfile(InputProfile, ProfileType, OutputProfile))
>     return -1;
>   return 0;

Done.


Thanks.  Diego.