Add -fauto-profile option to Clang driver

[Chandler Carruth]

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 <hans at chromium.org> wrote:

> On Tue, Oct 22, 2013 at 6:56 AM, Diego Novillo <dnovillo at google.com>
> wrote:
> > On Thu, Oct 10, 2013 at 2:48 PM, Hans Wennborg <hans at chromium.org>
> wrote:
> >> On Mon, Oct 7, 2013 at 11:09 AM, Diego Novillo <dnovillo at google.com>
> 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 <dnovillo at google.com>
> >>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131022/04a51234/attachment.html>