<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Oct 29, 2013, at 2:39 PM, Diego Novillo <<a href="mailto:dnovillo@google.com">dnovillo@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Mon, Oct 28, 2013 at 11:46 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br><blockquote type="cite">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">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a><br></blockquote><br>Done.<br><br><blockquote type="cite">+ // 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></blockquote><br>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></blockquote><div><br></div><div>No there’s not, and it really sucks. There’s been a bit of recent motion on doing something about that in other contexts, though. If you’re interested in pursuing that at some point, I suggest chatting with Quentin Colombet, who’s been doing some investigations on how to go about it.</div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">+ 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></blockquote><br>Done.<br><br><blockquote type="cite">if (changed) dumpInstrumentedInsns<br>No space between if and (changed).<br></blockquote><br>Hm? This is the exact opposite of what the coding conventions request<br>(<a href="http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses">http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses</a>)<br><br><blockquote type="cite">+ 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></blockquote><br>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><br><blockquote type="cite">+ ; 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></blockquote><br>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><br><blockquote type="cite">Also no C code for<br>this example? It's fine if you just constructed it, but I'm curious.<br></blockquote><br>I re-made the C code snippet from the bitcode.<br><br><blockquote type="cite">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></blockquote><br>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><span><0001-AutoProfile-pass.-Initial-setup.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></body></html>