Use auto profiler to set branch weights

[Eric Christopher]

>
> > +    // 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.
>
> llvm_unreachable is too drastic. This is a user error. I changed it to
> report_fatal_error(), but I'm not sure if I like it.  Is there any
> other way to indicate a user error in the backend?
>

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.


> > if (changed) dumpInstrumentedInsns
> > No space between if and (changed).
>
> Hm?  This is the exact opposite of what the coding conventions request
> (http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses)
>

Brain fart. Sorry :)


>
> > +  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?
>
> That would require removing all the support in the base class, which
> defines the interface.  Is it too much of a problem to just keep this?
>  My next patch will need to use this interface (I'm adding the bitcode
> reader next).
>
>
OK.


> > +  ; 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...?
>
> I clarified the comment.  It is a limitation in sampling. Since the
> branch is assigned the same line number as the increment operation
> (they are on the same line after all), the profiler thinks that it is
> sampled 288 times.  However, using discriminators or column number
> information, we should be able to tell them apart.
>
>
*nod* Have you tried turning on column information via the front end?


> > Also no C code for
> > this example? It's fine if you just constructed it, but I'm curious.
>
> I re-made the C code snippet from the bitcode.
>

Cool.


>
> > 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.
>
> What string?  The filename?  How else would I pass it to the backend?
> I don't understand what you mean with 'having a command line variable
> that contains the string'.
>
>
Chandler would probably kill me but I meant something like:

  cl::opt<string> InputFilename...

that's then passed by the front end. I'd rather it be part of the pass
constructor myself where you've got it.



> Here's the revised patch.
>

 Looks pretty good. Couple random things:

dumpInstrumentedInsns: I'd probably make this printInstrumentedInsns with a
dump that defaults to dbgs() etc.

Missed a few names ;)

+      unsigned line_offset = atoi(Matches[1].str().c_str());
+  StringRef name = F.getName();
+      SmallVector<Value *, 1> sample_values;
+  LLVMContext &context = FirstInst.getContext();

Using the regex support to read the file is exciting. Curious why? :)

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131029/d99107a6/attachment.html>