<div dir="ltr">I would *vastly* prefer to review this in Phabricator. I'm just too slow with patch files.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 29, 2013 at 2:39 PM, 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"><div class="im">On Mon, Oct 28, 2013 at 11:46 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
> Some superficial notes:<br>
><br>
> doxygen based documentation would be good for this.<br>
><br>
> +#include <cstdlib><br>
><br>
> You don't appear to use this and we don't generally use this anyhow :)<br>
><br>
> +// TODO(dnovillo) - Eventually this class ought to move to a separate file.<br>
><br>
> We don't usually put names.<br>
><br>
> + std::string filename_;<br>
><br>
> No underscores at the end of variable names.<br>
><br>
> + void dumpFunctionProfile(StringRef fn_name);<br>
><br>
> Or in the middle. :)<br>
><br>
> There are more than a few of these cases. You just want to read the naming<br>
> convention here:<br>
> <a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a><br>
<br>
</div>Done.<br>
<div class="im"><br>
> + // TODO(dnovillo) - This is almost certainly the wrong way to emit<br>
> + // diagnostics and exit the compiler.<br>
> + errs() << filename_ << ":" << linenum_ << ": " << msg << "\n";<br>
> + exit(1);<br>
><br>
> The TODO is correct. Definitely don't want to exit(1) here. You may as well<br>
> just turn this into llvm_unreachable.<br>
<br>
</div>llvm_unreachable is too drastic. This is a user error. I changed it to<br>
report_fatal_error(), but I'm not sure if I like it. Is there any<br>
other way to indicate a user error in the backend?<br>
<div class="im"><br>
> + errs() << "Function: " << fn_name << ", " << fn_profile.TotalSamples <<<br>
> ", "<br>
> + << fn_profile.TotalHeadSamples << ", " <<<br>
> fn_profile.BodySamples.size()<br>
> + << " sampled lines\n";<br>
><br>
> This should probably take the stream that you want to dump to and output to<br>
> that. An example might be the print and dump functions in<br>
> lib/IR/DebugInfo.cpp.<br>
><br>
> + errs() << "\n\nInstructions changed in " << F.getName() << "\n";<br>
><br>
> This should be dbgs() not errs(). Though it could also take a stream.<br>
<br>
</div>Done.<br>
<div class="im"><br>
> if (changed) dumpInstrumentedInsns<br>
> No space between if and (changed).<br>
<br>
</div>Hm? This is the exact opposite of what the coding conventions request<br>
(<a href="http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses" target="_blank">http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses</a>)<br>
<div class="im"><br>
> + return (is_text) ? profiler_->loadText() : profiler_->loadNative();<br>
><br>
> Given that loadNative isn't implemented how about just removing that part of<br>
> this including isText for now?<br>
<br>
</div>That would require removing all the support in the base class, which<br>
defines the interface. Is it too much of a problem to just keep this?<br>
My next patch will need to use this interface (I'm adding the bitcode<br>
reader next).<br>
<div class="im"><br>
> + ; This branch is really executed once. But the line number assignments<br>
><br>
> "once, but"<br>
><br>
> + ; TODO(dnovillo) - will need to adjust things like this.<br>
><br>
> Adjust for..? Is this actually a bug at the moment or...?<br>
<br>
</div>I clarified the comment. It is a limitation in sampling. Since the<br>
branch is assigned the same line number as the increment operation<br>
(they are on the same line after all), the profiler thinks that it is<br>
sampled 288 times. However, using discriminators or column number<br>
information, we should be able to tell them apart.<br>
<div class="im"><br>
> Also no C code for<br>
> this example? It's fine if you just constructed it, but I'm curious.<br>
<br>
</div>I re-made the C code snippet from the bitcode.<br>
<div class="im"><br>
> The structure of the pass seems ok. It's a bit weird for the pass to take a<br>
> string argument, but perhaps that's better than having a command line<br>
> variable that contains the string. I don't have a strong feeling one way or<br>
> another here.<br>
<br>
</div>What string? The filename? How else would I pass it to the backend?<br>
I don't understand what you mean with 'having a command line variable<br>
that contains the string'.<br>
<br>
Here's the revised patch.<br>
<br>
<br>
Thanks. Diego.<br>
</blockquote></div><br></div>