My only objection to this patch is the use of the word "auto". ;] While I know there is some historical precedent, I think that if this is going into mainline Clang, it would be worthwhile to strive for a more descriptive name. '-fsample-profile'? Other ideas?
On Tue, Oct 22, 2013 at 10:02 AM, Hans Wennborg <[email protected]> wrote: > On Tue, Oct 22, 2013 at 6:56 AM, Diego Novillo <[email protected]> > wrote: > > On Thu, Oct 10, 2013 at 2:48 PM, Hans Wennborg <[email protected]> > wrote: > >> On Mon, Oct 7, 2013 at 11:09 AM, Diego Novillo <[email protected]> > wrote: > >>> This adds a new option -fauto-profile=filename to Clang. It tells the > >>> driver to schedule the auto-profile pass and passes on the name of the > >>> profile file to use. > >>> > >>> This patch depends on the initial auto profile patch I posted a > >>> couple of weeks ago: > http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130923/188838.html > >> > >> This looks good as far as I can tell, just minor comments: > >> > >>> From bcfe88d501ad4ea299f0b64ce98b375c248a45c9 Mon Sep 17 00:00:00 2001 > >>> From: Diego Novillo <[email protected]> > >>> Date: Wed, 2 Oct 2013 10:46:34 -0400 > >>> Subject: [PATCH] Add -fauto-profile to Clang's driver. > >>> > >>> This adds a new option -fauto-profile=filename to Clang. It tells the > >>> driver to schedule the auto-profile pass and passes on the name of the > >>> profile file to use. > >> > >>> +++ b/lib/CodeGen/BackendUtil.cpp > >>> @@ -154,6 +154,14 @@ static void addObjCARCOptPass(const > PassManagerBuilder &Builder, PassManagerBase > >>> PM.add(createObjCARCOptPass()); > >>> } > >>> > >>> +static void addAutoProfilePass(const PassManagerBuilder &Builder, > >>> + PassManagerBase &PM) { > >>> + const PassManagerBuilderWrapper &BuilderWrapper = > >>> + static_cast<const PassManagerBuilderWrapper&>(Builder); > >>> + const CodeGenOptions &opts = BuilderWrapper.getCGOpts(); > >> > >> I'd have called the variable CGOpts, but whatever. > >> > >>> +++ b/lib/Driver/Tools.cpp > >>> @@ -3020,6 +3020,10 @@ void Clang::ConstructJob(Compilation &C, const > JobAction &JA, > >>> > >>> // Forward -f options with positive and negative forms; we translate > >>> // these by hand. > >>> + if (Arg *A = Args.getLastArg(options::OPT_fauto_profile_EQ)) { > >>> + StringRef fname = A->getValue(); > >>> + CmdArgs.push_back(Args.MakeArgString("-fauto-profile=" + fname)); > >> > >> I think you can just use A->render(Args, CmdArgs); here, saving you > >> the trouble of doing the MakeArgString dance. > >> > >> Would this be a good point to check for the existence of the file, or > >> what's the plan there? > > > > Here's an updated patch with the suggestions above. OK to commit? > > LGTM with comments: > > > --- a/lib/Driver/Tools.cpp > > +++ b/lib/Driver/Tools.cpp > > @@ -3034,6 +3034,12 @@ void Clang::ConstructJob(Compilation &C, const > JobAction &JA, > > > > // Forward -f options with positive and negative forms; we translate > > // these by hand. > > + if (Arg *A = Args.getLastArg(options::OPT_fauto_profile_EQ)) { > > + StringRef fname = A->getValue(); > > + if (!llvm::sys::fs::exists(fname)) > > + D.Diag(diag::err_drv_no_such_file) << fname; > > + A->render(Args, CmdArgs); > > + } > > I don't think we should forward the flag to CC1 if the file doesn't > exist. And maybe we could use DiagnoseInputExistance() in Driver.cpp > to perform the check - that's the function used to check for regular > input files. > > > +++ b/test/Driver/clang_f_opts.c > > @@ -44,6 +44,9 @@ > > // CHECK-UNROLL-LOOPS: "-funroll-loops" > > // CHECK-NO-UNROLL-LOOPS: "-fno-unroll-loops" > > > > +// RUN: %clang -### -S -fauto-profile=file.prof %s 2>&1 | FileCheck > -check-prefix=CHECK-AUTO-PROFILE %s > > +// CHECK-AUTO-PROFILE: "-fauto-profile=file.prof" > > Won't this trigger an error since file.prof doesn't exist? You could > put an empty file.prof in test/Driver/Inputs. > > Cheers, > Hans >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
