[PATCH] Add profile writing capabilities for sampling profiles.

[Diego Novillo]

On 27/10/2014, 13:49 , Justin Bogner wrote:
> +namespace sampleprof {
> Should this be nested to llvm::sampleprof? I don't think we usually use
> top-level namespaces other than llvm::.

Done.

>> +  /// \brief Add called function \p F with samples \p S.
>> +  void addCalledTarget(std::string F, unsigned S) {
>> +    CallTargets.push_back(std::pair<std::string, unsigned>(F, S));
> Can get rid of some boilerplate here:
>
>      CallTargets.push_back(std::make_pair(F, S));
>
> Unfortunately SmallVector doesn't have emplace_back, as that would be
> even nicer.

Done.

>   
>   using namespace llvm;
> This isn't new, but "using namespace" in a header is a pretty bad idea
> in general. Please fix this, though feel free to do it as a separate
> commit if that's more appropriate.

Done.
>     /// \brief Report a parse error message.
>     void reportParseError(int64_t LineNumber, Twine Msg) const {
> -    DiagnosticInfoSampleProfile Diag(Filename.data(), LineNumber, Msg);
> -    M.getContext().diagnose(Diag);
> +    if (M) {
> +      DiagnosticInfoSampleProfile Diag(Buffer->getBufferIdentifier(),
> +                                       LineNumber, Msg);
> +      M->getContext().diagnose(Diag);
> +    } else
> +      errs() << Msg << "\n";
> This seems like a hack. Why is M allowed to be null?

When I use this from llvm-profdata, I won't have a Module to use for 
diagnostics. Or maybe I'm just confused? As you say below, I only really 
need the LLVMContext instance. Do I have access to it from llvm-profdata?

> +  /// LLVM context for diagnostics. If this is nil, messages are emitted
> +  /// to errs().
> +  const Module *M;
> Again, why do we need the errs() behaviour? Also, AFAICT (and according
> to the comment) M is only used for M.getContext() - why not store the
> "const LLVMContext &" instead?

Yeah, using LLVMContext is a better alternative. My concern is similar, 
however. Will I have an LLVMContext in llvm-profdata?

> +
> +  virtual bool write(const Function &F, const FunctionSamples &S) override;
> The LLVM style usually omits the virtual keyword when override is
> present, since it's redundant.

Done.

> +       SI != SE; ++SI) {
> +    LineLocation Loc = SI->first;
> +    SampleRecord Sample = SI->second;
> +    OS << "\tline offset: " << Loc.LineOffset
> +       << ", discriminator: " << Loc.Discriminator
> +       << ", number of samples: " << Sample.getSamples();
> +    if (Sample.hasCalls()) {
> The handling of calls seems like a separate change from adding the
> binary format. Can you break it out into its own commit?

Yeah, but if it's OK with you, it would be a bit more convenient if I 
can leave this in.  This affects the tests that I added for the binary 
writer. I'd have to remove it, only to add it immediately after.

> +  static bool hacked = false;
> +  if (!hacked && getenv("PROF_WRITER")) {
> +    hacked = true;
> +    std::error_code EC;
> +    const char *txt = "/tmp/kk.prof.txt";
> +    const char *bin = "/tmp/kk.prof";
> I guess this is some debug code you left in by accident?

Yeah. I was convinced I had removed it. Sigh.

> -/// \brief Load execution samples from a file.
> +template <typename T>
> +T SampleProfileReaderBinary::readNumber(std::error_code *EC) {
> It *might* be worth changing this to return ErrorOr<T> instead of having
> an out parameter. It won't really make the call sites much prettier, but
> it (arguably) makes it harder to accidentally ignore the error.

Yeah, I thought about it, but it didn't read better for me.  You're 
right in that it's harder to accidentally ignore errors. I'll change it.

> +      // When dealing with instruction weights, we use the value
> +      // zero to indicate the absence of a sample. If we read an
> +      // actual zero from the profile file, return it as 1 to
> +      // avoid the confusion later on.
> +      if (NumSamples == 0)
> +        NumSamples = 1;
> Should addBodySamples handle this, instead of duplicating the logic and
> comment between the two readers?

Done.

> +/// \returns an error code indicating the status of the buffer.
> +static std::error_code
> +setupMemoryBuffer(std::string Filename, std::unique_ptr<MemoryBuffer> &Buffer) {
> +  ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
> +      MemoryBuffer::getFileOrSTDIN(Filename);
> auto's appropriate here.

Done.

> +// This file implements the class that writes LLVM sample profiles. It
> +// supports two file formats: text and bitcode. The textual representation
> +// is useful for debugging and testing purposes. The bitcode representation
> +// is more compact, resulting in smaller file sizes. However, they can
> +// both be used interchangeably.
> Looks like you missed s/bitcode/binary/ here.

Done.


Thanks. Diego.