Use auto profiler to set branch weights

[Chandler Carruth]

I would *vastly* prefer to review this in Phabricator. I'm just too slow
with patch files.


On Tue, Oct 29, 2013 at 2:39 PM, Diego Novillo <dnovillo at google.com> wrote:

> On Mon, Oct 28, 2013 at 11:46 AM, Eric Christopher <echristo at gmail.com>
> wrote:
> > Some superficial notes:
> >
> > doxygen based documentation would be good for this.
> >
> > +#include <cstdlib>
> >
> > You don't appear to use this and we don't generally use this anyhow :)
> >
> > +// TODO(dnovillo) - Eventually this class ought to move to a separate
> file.
> >
> > We don't usually put names.
> >
> > +  std::string filename_;
> >
> > No underscores at the end of variable names.
> >
> > +  void dumpFunctionProfile(StringRef fn_name);
> >
> > Or in the middle. :)
> >
> > There are more than a few of these cases. You just want to read the
> naming
> > convention here:
> >
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
>
> Done.
>
> > +    // 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?
>
> > +  errs() << "Function: " << fn_name << ", " << fn_profile.TotalSamples
> <<
> > ", "
> > +         << fn_profile.TotalHeadSamples << ", " <<
> > fn_profile.BodySamples.size()
> > +         << " sampled lines\n";
> >
> > 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.
> >
> > +  errs() << "\n\nInstructions changed in " << F.getName() << "\n";
> >
> > This should be dbgs() not errs(). Though it could also take a stream.
>
> Done.
>
> > 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)
>
> > +  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).
>
> > +  ; 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.
>
> > 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.
>
> > 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'.
>
> Here's the revised patch.
>
>
> Thanks.  Diego.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131029/4acf1c57/attachment.html>