<div dir="ltr">Some superficial notes:<div><br></div><div>doxygen based documentation would be good for this.<br><div><br></div><div><div>+#include <cstdlib></div></div><div><br></div><div>You don't appear to use this and we don't generally use this anyhow :)</div>
<div><br></div><div><div>+// TODO(dnovillo) - Eventually this class ought to move to a separate file.</div></div><div><br></div><div>We don't usually put names.</div><div><br></div><div><div>+  std::string filename_;</div>
</div><div><br></div><div>No underscores at the end of variable names.</div><div><br></div><div><div>+  void dumpFunctionProfile(StringRef fn_name);</div></div><div><br></div><div>Or in the middle. :)</div><div><br></div>
</div><div>There are more than a few of these cases. You just want to read the naming convention here: <a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a></div>
<div><br></div><div><div>+    // TODO(dnovillo) - This is almost certainly the wrong way to emit</div><div>+    // diagnostics and exit the compiler.</div><div>+    errs() << filename_ << ":" << linenum_ << ": " << msg << "\n";</div>
<div>+    exit(1);</div></div><div><br></div><div>The TODO is correct. Definitely don't want to exit(1) here. You may as well just turn this into llvm_unreachable.</div><div><br></div><div><div>+  errs() << "Function: " << fn_name << ", " << fn_profile.TotalSamples << ", "</div>
<div>+         << fn_profile.TotalHeadSamples << ", " << fn_profile.BodySamples.size()</div><div>+         << " sampled lines\n";</div></div><div><br></div><div>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.</div>
<div><br></div><div><div>+  errs() << "\n\nInstructions changed in " << F.getName() << "\n";</div></div><div><br></div><div>This should be dbgs() not errs(). Though it could also take a stream.</div>
<div><br></div><div>if (changed) dumpInstrumentedInsns<br></div><div>No space between if and (changed).</div><div><br></div><div><div>+  return (is_text) ? profiler_->loadText() : profiler_->loadNative();</div></div>
<div><br></div><div>Given that loadNative isn't implemented how about just removing that part of this including isText for now?</div><div><br></div><div><div>+  ; This branch is really executed once. But the line number assignments</div>
</div><div><br></div><div>"once, but"</div><div><br></div><div><div>+  ; TODO(dnovillo) - will need to adjust things like this.</div></div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>-eric</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 28, 2013 at 8:21 AM, Diego Novillo <span dir="ltr"><<a href="mailto:dnovillo@google.com" target="_blank">dnovillo@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ping?  This is starting to block my progress.<br>
<br>
<br>
The attached patch includes Eric's comments on the test case.  I still<br>
have not renamed the option names, as we have not reached closure on<br>
the flag names.<br>
<br>
The implementation works well enough to produce SPECint results. I've<br>
compared GCC's implementation of the auto profiler vs mine:<br>
<br>
- GCC gets performance differences between -2% and +14%, with the mean<br>
improving by +4%<br>
- LLVM gets performance differences between -7% and +2%, with the mean<br>
degrading by -1%<br>
<br>
I'm not too concerned (yet) about the bad performance I'm getting with<br>
LLVM. Both compilers can build all of SPECint (I'm only having trouble<br>
with perlbench) and, clearly, the branch hints generated by the patch<br>
are having an effect.<br>
<br>
I will use spec and other performance benchmarks to drive further<br>
changes to the LLVM implementation. Right now I'm interested in a<br>
review of the actual workings of the patch.  I am sure that it will<br>
need changes, and I would like to get those done before I start<br>
completing it to get the performance numbers up.<br>
<br>
<br>
Thanks.  Diego.<br>
</blockquote></div><br></div>