[PATCH] Use auto profiler to set branch weights

Diego Novillo dnovillo at google.com
Tue Nov 12 09:09:21 PST 2013


On Tue, Nov 12, 2013 at 7:44 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>
>   Comments from the peanut gallery.

Thanks!

> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:314
> @@ +313,3 @@
> +    Loader.reportParseError("Expected a number, found " + Line);
> +  int NumSymbols = atoi(Line.c_str());
> +  for (int I = 0; I < NumSymbols; I++) {
> ----------------
> I think we prefer StringRef's getAsInteger to atoi. It also does error checking so you don't have to do it with a regex. That would also eliminate the std::string conversions above.

Done.

>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:336
> @@ +335,3 @@
> +    StringRef FName = Matches[1];
> +    unsigned NumSamples = atoi(Matches[2].str().c_str());
> +    unsigned NumHeadSamples = atoi(Matches[3].str().c_str());
> ----------------
> More atoi()s.

Done.

>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:377
> @@ +376,3 @@
> +  unsigned LOffset = Inst.getDebugLoc().getLine() - FirstLineno + 1;
> +  return (BodySamples.find(LOffset) != BodySamples.end()) ? BodySamples[LOffset]
> +                                                          : 0;
> ----------------
> This is equivalent to BodySamples.lookup(LOffset)

Done.

>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:435
> @@ +434,3 @@
> +  unsigned FirstLineno = FirstInst.getDebugLoc().getLine();
> +  llvm::MDBuilder MDB(F.getContext());
> +
> ----------------
> Why the extra llvm:: qualification? We're in a using namespace llvm environment anyways.

Fixed.

I've uploaded a revised version of the patch.


Thanks.  Diego.



More information about the llvm-commits mailing list