Add -fauto-profile option to Clang driver

[Hans Wennborg]

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