<div dir="ltr">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?</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 22, 2013 at 10:02 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Tue, Oct 22, 2013 at 6:56 AM, Diego Novillo <<a href="mailto:dnovillo@google.com">dnovillo@google.com</a>> wrote:<br>
> On Thu, Oct 10, 2013 at 2:48 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>> On Mon, Oct 7, 2013 at 11:09 AM, Diego Novillo <<a href="mailto:dnovillo@google.com">dnovillo@google.com</a>> wrote:<br>
>>> This adds a new option -fauto-profile=filename to Clang. It tells the<br>
>>> driver to schedule the auto-profile pass and passes on the name of the<br>
>>> profile file to use.<br>
>>><br>
>>> This patch depends on the initial auto profile patch I posted a<br>
>>> couple of weeks ago: <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130923/188838.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130923/188838.html</a><br>

>><br>
>> This looks good as far as I can tell, just minor comments:<br>
>><br>
>>> From bcfe88d501ad4ea299f0b64ce98b375c248a45c9 Mon Sep 17 00:00:00 2001<br>
>>> From: Diego Novillo <<a href="mailto:dnovillo@google.com">dnovillo@google.com</a>><br>
>>> Date: Wed, 2 Oct 2013 10:46:34 -0400<br>
>>> Subject: [PATCH] Add -fauto-profile to Clang's driver.<br>
>>><br>
>>> This adds a new option -fauto-profile=filename to Clang. It tells the<br>
>>> driver to schedule the auto-profile pass and passes on the name of the<br>
>>> profile file to use.<br>
>><br>
>>> +++ b/lib/CodeGen/BackendUtil.cpp<br>
>>> @@ -154,6 +154,14 @@ static void addObjCARCOptPass(const PassManagerBuilder &Builder, PassManagerBase<br>
>>>      PM.add(createObjCARCOptPass());<br>
>>>  }<br>
>>><br>
>>> +static void addAutoProfilePass(const PassManagerBuilder &Builder,<br>
>>> +                               PassManagerBase &PM) {<br>
>>> +  const PassManagerBuilderWrapper &BuilderWrapper =<br>
>>> +      static_cast<const PassManagerBuilderWrapper&>(Builder);<br>
>>> +  const CodeGenOptions &opts = BuilderWrapper.getCGOpts();<br>
>><br>
>> I'd have called the variable CGOpts, but whatever.<br>
>><br>
>>> +++ b/lib/Driver/Tools.cpp<br>
>>> @@ -3020,6 +3020,10 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,<br>
>>><br>
>>>    // Forward -f options with positive and negative forms; we translate<br>
>>>    // these by hand.<br>
>>> +  if (Arg *A = Args.getLastArg(options::OPT_fauto_profile_EQ)) {<br>
>>> +    StringRef fname = A->getValue();<br>
>>> +    CmdArgs.push_back(Args.MakeArgString("-fauto-profile=" + fname));<br>
>><br>
>> I think you can just use A->render(Args, CmdArgs); here, saving you<br>
>> the trouble of doing the MakeArgString dance.<br>
>><br>
>> Would this be a good point to check for the existence of the file, or<br>
>> what's the plan there?<br>
><br>
> Here's an updated patch with the suggestions above.  OK to commit?<br>
<br>
</div></div>LGTM with comments:<br>
<br>
> --- a/lib/Driver/Tools.cpp<br>
> +++ b/lib/Driver/Tools.cpp<br>
> @@ -3034,6 +3034,12 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,<br>
<div class="im">><br>
>    // Forward -f options with positive and negative forms; we translate<br>
>    // these by hand.<br>
> +  if (Arg *A = Args.getLastArg(options::OPT_fauto_profile_EQ)) {<br>
> +    StringRef fname = A->getValue();<br>
</div>> +    if (!llvm::sys::fs::exists(fname))<br>
> +      D.Diag(diag::err_drv_no_such_file) << fname;<br>
> +    A->render(Args, CmdArgs);<br>
> +  }<br>
<br>
I don't think we should forward the flag to CC1 if the file doesn't<br>
exist. And maybe we could use DiagnoseInputExistance() in Driver.cpp<br>
to perform the check - that's the function used to check for regular<br>
input files.<br>
<br>
> +++ b/test/Driver/clang_f_opts.c<br>
> @@ -44,6 +44,9 @@<br>
>  // CHECK-UNROLL-LOOPS: "-funroll-loops"<br>
>  // CHECK-NO-UNROLL-LOOPS: "-fno-unroll-loops"<br>
><br>
> +// RUN: %clang -### -S -fauto-profile=file.prof %s 2>&1 | FileCheck -check-prefix=CHECK-AUTO-PROFILE %s<br>
> +// CHECK-AUTO-PROFILE: "-fauto-profile=file.prof"<br>
<br>
Won't this trigger an error since file.prof doesn't exist? You could<br>
put an empty file.prof in test/Driver/Inputs.<br>
<br>
Cheers,<br>
Hans<br>
</blockquote></div><br></div>