Use auto profiler to set branch weights

[Diego Novillo]

On Mon, Oct 28, 2013 at 11:46 AM, Eric Christopher <echristo at gmail.com> wrote:
> Some superficial notes:
>
> doxygen based documentation would be good for this.
>
> +#include <cstdlib>
>
> You don't appear to use this and we don't generally use this anyhow :)
>
> +// TODO(dnovillo) - Eventually this class ought to move to a separate file.
>
> We don't usually put names.
>
> +  std::string filename_;
>
> No underscores at the end of variable names.
>
> +  void dumpFunctionProfile(StringRef fn_name);
>
> Or in the middle. :)
>
> There are more than a few of these cases. You just want to read the naming
> convention here:
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Done.

> +    // TODO(dnovillo) - This is almost certainly the wrong way to emit
> +    // diagnostics and exit the compiler.
> +    errs() << filename_ << ":" << linenum_ << ": " << msg << "\n";
> +    exit(1);
>
> The TODO is correct. Definitely don't want to exit(1) here. You may as well
> just turn this into llvm_unreachable.

llvm_unreachable is too drastic. This is a user error. I changed it to
report_fatal_error(), but I'm not sure if I like it.  Is there any
other way to indicate a user error in the backend?

> +  errs() << "Function: " << fn_name << ", " << fn_profile.TotalSamples <<
> ", "
> +         << fn_profile.TotalHeadSamples << ", " <<
> fn_profile.BodySamples.size()
> +         << " sampled lines\n";
>
> This should probably take the stream that you want to dump to and output to
> that. An example might be the print and dump functions in
> lib/IR/DebugInfo.cpp.
>
> +  errs() << "\n\nInstructions changed in " << F.getName() << "\n";
>
> This should be dbgs() not errs(). Though it could also take a stream.

Done.

> if (changed) dumpInstrumentedInsns
> No space between if and (changed).

Hm?  This is the exact opposite of what the coding conventions request
(http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses)

> +  return (is_text) ? profiler_->loadText() : profiler_->loadNative();
>
> Given that loadNative isn't implemented how about just removing that part of
> this including isText for now?

That would require removing all the support in the base class, which
defines the interface.  Is it too much of a problem to just keep this?
 My next patch will need to use this interface (I'm adding the bitcode
reader next).

> +  ; This branch is really executed once. But the line number assignments
>
> "once, but"
>
> +  ; TODO(dnovillo) - will need to adjust things like this.
>
> Adjust for..? Is this actually a bug at the moment or...?

I clarified the comment.  It is a limitation in sampling. Since the
branch is assigned the same line number as the increment operation
(they are on the same line after all), the profiler thinks that it is
sampled 288 times.  However, using discriminators or column number
information, we should be able to tell them apart.

> Also no C code for
> this example? It's fine if you just constructed it, but I'm curious.

I re-made the C code snippet from the bitcode.

> The structure of the pass seems ok. It's a bit weird for the pass to take a
> string argument, but perhaps that's better than having a command line
> variable that contains the string. I don't have a strong feeling one way or
> another here.

What string?  The filename?  How else would I pass it to the backend?
I don't understand what you mean with 'having a command line variable
that contains the string'.

Here's the revised patch.


Thanks.  Diego.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AutoProfile-pass.-Initial-setup.patch
Type: application/octet-stream
Size: 41926 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131029/b41b39af/attachment.obj>