<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<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></blockquote><div><br></div><div>exit(1) is bad no matter what. llvm is a library, not an executable. report_fatal_error is about the best you get. I.e. if it's likely user/tool error then report_fatal_error if it should never happen then llvm_unreachable.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
> 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></blockquote><div><br></div><div>Brain fart. Sorry :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<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></div></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + ; 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></div></blockquote><div><br></div><div>*nod* Have you tried turning on column information via the front end?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> 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></blockquote><div><br></div><div>Cool.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<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></blockquote><div><br></div><div>Chandler would probably kill me but I meant something like:</div><div><br></div><div><div> cl::opt<string> InputFilename...</div></div><div><br></div><div>that's then passed by the front end. I'd rather it be part of the pass constructor myself where you've got it.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Here's the revised patch.<br></blockquote><div><br></div><div> Looks pretty good. Couple random things:</div><div><br></div><div>dumpInstrumentedInsns: I'd probably make this printInstrumentedInsns with a dump that defaults to dbgs() etc.</div>
<div><br></div><div>Missed a few names ;)</div><div><br></div><div>+ unsigned line_offset = atoi(Matches[1].str().c_str());<br></div><div>+ StringRef name = F.getName();<br></div><div>+ SmallVector<Value *, 1> sample_values;<br>
</div><div>+ LLVMContext &context = FirstInst.getContext();<br></div><div><br></div><div>Using the regex support to read the file is exciting. Curious why? :)</div><div><br></div><div>-eric</div></div></div></div>