[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