Use auto profiler to set branch weights

[Jim Grosbach]

On 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?

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.

> 
>> +  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.
> <0001-AutoProfile-pass.-Initial-setup.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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