[PATCH] Add profile writing capabilities for sampling profiles.

[Justin Bogner]

Diego Novillo <dnovillo at google.com> writes:
> 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?

I think you can just getGlobalContext() in llvm-profdata and pass that
in. Other tools do something similar.

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