[PATCH] This patch is the full version of the 5 individual patches that portthe Perf converter from https://github.com/google/autofdo.The earlier patches are described in detail at:http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon...

[Justin Bogner]

Diego Novillo <dnovillo at google.com> writes:
> I tried creating 5 individual phabricator issues for this, but they are
> very hard to maintain as they are all in the same tree. Phabricator does
> not make this easy in any way.
>
> Main changes:
>
> - Remove unused parts of LinuxPerf/utils.cc to avoid -lcrypto dependency.
> - Add -Wno-pedantic for LinuxPerf
> - Removed all the unused code.
> - Removed all traces of base/, glog/ and gflags/
> - Add samplepgo namespace. Rename ProfileCreator to SampleProfileConverter.
> - Add LLVM copyright headers.
> - Add support for autoconf.
>
> http://reviews.llvm.org/D4850

I haven't looked at anything under ProfileData/PerfConverter in detail,
but it looks pretty reasonable at a high level given that you plan on
replacing the parts that obviously duplicate llvm functionality RSN.
Some tests would be nice.

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.

The changes to llvm-profdata are obvious and look great. I have a minor
style comment in there, but otherwise that part LGTM.

> Index: include/llvm/ProfileData/SampleProfileConverter.h
> ===================================================================
> --- /dev/null
> +++ include/llvm/ProfileData/SampleProfileConverter.h
> @@ -0,0 +1,57 @@
> +//=-- SampleProfileConverter.h - Sample profile converter API -----*- C++ -*-=//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file contains support for converting profiles from external profilers
> +// into the format supported by lib/Transforms/Scalar/SampleProfile.cpp.
> +//
> +//===----------------------------------------------------------------------===//
> +#ifndef SAMPLEPGO_PROFILE_CONVERTER_H_
> +#define SAMPLEPGO_PROFILE_CONVERTER_H_
> +
> +#include "llvm/ProfileData/SampleProfileReader.h"
> +
> +namespace samplepgo {
> +
> +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.

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

> +
> +  // Creates SamplePGO profile, returns true if success, false otherwise.

Please add an extra slash (///) so that doxygen picks these up.

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

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

> +
> + private:
> +  SampleReader *sample_reader_;
> +  string binary_;
> +};
> +
> +}  // namespace samplepgo
> +
> +#endif  // SAMPLEPGO_PROFILE_CONVERTER_H_
> Index: include/llvm/ProfileData/SampleProfileReader.h
> ===================================================================
> --- /dev/null
> +++ include/llvm/ProfileData/SampleProfileReader.h
> @@ -0,0 +1,106 @@
> +//=-- SampleProfileReader.h - Generic sample profiling reader API -*- C++ -*-=//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file contains support for reading profiling data from external
> +// sampling profilers.
> +//
> +//===----------------------------------------------------------------------===//
> +#ifndef SAMPLEPGO_SAMPLE_READER_H_
> +#define SAMPLEPGO_SAMPLE_READER_H_
> +
> +#include <map>
> +#include <set>
> +#include <string>
> +#include <vector>
> +#include <utility>
> +
> +#include "llvm/Support/DataTypes.h"
> +
> +using namespace std;
> +
> +namespace samplepgo {
> +
> +typedef map<uint64_t, uint64_t> AddressCountMap;
> +typedef pair<uint64_t, uint64_t> Range;
> +typedef map<Range, uint64_t> RangeCountMap;
> +typedef pair<uint64_t, uint64_t> Branch;
> +typedef map<Branch, uint64_t> BranchCountMap;
> +
> +// Reads in the profile data, and represent it in address_count_map_.
> +class SampleReader {
> +public:
> +  explicit SampleReader() : max_count_(0) {}
> +  virtual ~SampleReader() {}
> +
> +  bool ReadAndSetMaxCount();
> +
> +  const AddressCountMap &address_count_map() const {
> +    return address_count_map_;
> +  }
> +
> +  const RangeCountMap &range_count_map() const { return range_count_map_; }
> +
> +  const BranchCountMap &branch_count_map() const { return branch_count_map_; }
> +
> +  set<uint64_t> GetSampledAddresses() const;
> +
> +  // Returns the sample count for a given instruction.
> +  uint64_t GetSampleCountOrZero(uint64_t addr) const;

See above comments on naming and doxygen comments :)

> +
> +  // Returns the total sampled count.
> +  uint64_t GetTotalSampleCount() const;
> +
> +  // Returns the max count.
> +  uint64_t GetMaxCount() const { return max_count_; }
> +
> +  // Clear all maps to release memory.
> +  void Clear() {
> +    address_count_map_.clear();
> +    range_count_map_.clear();
> +    branch_count_map_.clear();
> +  }
> +
> +protected:
> +  // Virtual read function to read from different types of profiles.
> +  virtual bool Read() = 0;
> +
> +  uint64_t max_count_;
> +  AddressCountMap address_count_map_;
> +  RangeCountMap range_count_map_;
> +  BranchCountMap branch_count_map_;
> +};
> +
> +// Base class that reads in the profile from a sample data file.
> +class FileSampleReader : public SampleReader {
> +public:
> +  explicit FileSampleReader(const string &profile_file)
> +      : profile_file_(profile_file) {}
> +
> +  virtual bool Append(const string &profile_file) = 0;
> +
> +protected:
> +  virtual bool Read();
> +
> +  string profile_file_;
> +};
> +
> +// Reads in the sample data from a 'perf -b' output file.
> +class PerfDataSampleReader : public FileSampleReader {
> +public:
> +  explicit PerfDataSampleReader(const string &profile_file, const string &re)
> +      : FileSampleReader(profile_file), focus_binary_re_(re) {}
> +  virtual bool Append(const string &profile_file);
> +
> +private:
> +  const string focus_binary_re_;
> +};
> +
> +} // namespace samplepgo
> +
> +#endif // SAMPLEPGO_SAMPLE_READER_H_

<snip>

> Index: tools/llvm-profdata/llvm-profdata.cpp
> ===================================================================
> --- tools/llvm-profdata/llvm-profdata.cpp
> +++ tools/llvm-profdata/llvm-profdata.cpp
> @@ -14,6 +14,7 @@
>  #include "llvm/ADT/StringRef.h"
>  #include "llvm/ProfileData/InstrProfReader.h"
>  #include "llvm/ProfileData/InstrProfWriter.h"
> +#include "llvm/ProfileData/SampleProfileConverter.h"
>  #include "llvm/Support/CommandLine.h"
>  #include "llvm/Support/FileSystem.h"
>  #include "llvm/Support/Format.h"
> @@ -150,6 +151,59 @@
>    return 0;
>  }
>
> +int convert_main(int argc, const char *argv[]) {
> +  cl::opt<std::string> InputProfile(cl::Positional, cl::Required,
> +                                    cl::desc("<profdata-file>"));
> +
> +  cl::opt<std::string> ProfileType(
> +      "profile-type", cl::desc("Input profile type"), cl::init("perf"));
> +
> +  cl::opt<std::string> OutputProfile("out",
> +                                     cl::desc("Output profile file name"));
> +  cl::alias OutputProfileA("o", cl::desc("Alias for --output"),
> +                           cl::aliasopt(OutputProfile));
> +
> +  cl::opt<std::string> InputBinary(
> +      "binary", cl::desc("Input binary executable (in ELF format) to read "
> +                         "(this executable must be compiled with -gmlt)"));
> +
> +  const char *usage = "\nConverts a sample profile collected with Linux Perf "
> +                      "(https://perf.wiki.kernel.org/)\n"
> +                      "into an LLVM sample profile. The output file can be "
> +                      "used with Clang's -fprofile-sample-use flag.\n"
> +                      "\nSample usage:\n"
> +                      "\n$ llvm-profdata --out=perf.llvm --binary=a.out "
> +                      "--type=perf perf.data\n";
> +
> +  cl::ParseCommandLineOptions(argc, argv, usage);
> +
> +  if (InputProfile.empty()) {
> +    errs() << usage;
> +    errs() << "Need a name for the input profile file.\n";
> +    return 1;
> +  }
> +
> +  if (OutputProfile.empty()) {
> +    errs() << usage;
> +    errs() << "Need a name for the output LLVM profile file.\n";
> +    errs() << "Use --out to specify an output file.\n";
> +    return 1;
> +  }
> +
> +  if (InputBinary.empty()) {
> +    errs() << usage;
> +    errs() << "Need a name for the ELF executable.\n";
> +    errs() << "Use --binary to specify an input executable file.\n";
> +    return 1;
> +  }
> +
> +  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;

> +}
> +
>  int main(int argc, const char *argv[]) {
>    // Print a stack trace if we signal out.
>    sys::PrintStackTraceOnErrorSignal();
> @@ -164,6 +218,8 @@
>        func = merge_main;
>      else if (strcmp(argv[1], "show") == 0)
>        func = show_main;
> +    else if (strcmp(argv[1], "convert") == 0)
> +      func = convert_main;
>
>      if (func) {
>        std::string Invocation(ProgName.str() + " " + argv[1]);
> @@ -178,7 +234,7 @@
>        errs() << "OVERVIEW: LLVM profile data tools\n\n"
>               << "USAGE: " << ProgName << " <command> [args...]\n"
>               << "USAGE: " << ProgName << " <command> -help\n\n"
> -             << "Available commands: merge, show\n";
> +             << "Available commands: merge, show, convert\n";
>        return 0;
>      }
>    }
> @@ -188,6 +244,6 @@
>    else
>      errs() << ProgName << ": Unknown command!\n";
>
> -  errs() << "USAGE: " << ProgName << " <merge|show> [args...]\n";
> +  errs() << "USAGE: " << ProgName << " <merge|show|convert> [args...]\n";
>    return 1;
>  }