Use auto profiler to set branch weights

[Eric Christopher]

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

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

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

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

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

+  ; 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...? Also no C code
for this example? It's fine if you just constructed it, but I'm curious.

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.

-eric




On Mon, Oct 28, 2013 at 8:21 AM, Diego Novillo <dnovillo at google.com> wrote:

> ping?  This is starting to block my progress.
>
>
> The attached patch includes Eric's comments on the test case.  I still
> have not renamed the option names, as we have not reached closure on
> the flag names.
>
> The implementation works well enough to produce SPECint results. I've
> compared GCC's implementation of the auto profiler vs mine:
>
> - GCC gets performance differences between -2% and +14%, with the mean
> improving by +4%
> - LLVM gets performance differences between -7% and +2%, with the mean
> degrading by -1%
>
> I'm not too concerned (yet) about the bad performance I'm getting with
> LLVM. Both compilers can build all of SPECint (I'm only having trouble
> with perlbench) and, clearly, the branch hints generated by the patch
> are having an effect.
>
> I will use spec and other performance benchmarks to drive further
> changes to the LLVM implementation. Right now I'm interested in a
> review of the actual workings of the patch.  I am sure that it will
> need changes, and I would like to get those done before I start
> completing it to get the performance numbers up.
>
>
> Thanks.  Diego.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131028/b11c7f9c/attachment.html>