[PATCH] Add profile writing capabilities for sampling profiles.
Diego Novillo
dnovillo at google.com
Mon Oct 27 17:48:33 PDT 2014
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.
More information about the llvm-commits
mailing list