[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

We first need to nail the use model of the option, and then talk about
implementation. To clarify, I think the following should be the way:

(assuming the current clang instrumetnation is the default):

1) To turn on clang based instrumentation:
  -fprofile-instr-generate[=<path>]
2) To turn on clang based instrumentation explicitly:
  -fprofile-instr-generate[=<path>] -Xclang -fprofile-instrumentor=clang

3) To turn on IR based instrumentation:
  -fprofile-instr-generate[=<path>] -Xclang -fprofile-instrumentor=llvm


There is no need to change driver handling -- just let it forward. In
CompilerInvocation.cpp, diagnostics can be emitted if user does

-Xclang -fprofile-instrumentor=xxx without specfiying -fprofile-instr-generate


Handling of -fprofile-instr-use=<> is different which does not depend
on -fprofile-instrumentor, so it should be done in a different patch.

David




On Wed, Jan 27, 2016 at 11:38 AM, Rong Xu <xur at google.com> wrote:
> On Wed, Jan 27, 2016 at 11:31 AM, David Li <davidxl at google.com> wrote:
>> davidxl added inline comments.
>>
>> ================
>> Comment at: lib/Driver/Tools.cpp:5520
>> @@ +5519,3 @@
>> +    A->claim();
>> +    if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
>> +        (Args.hasArg(options::OPT_fprofile_instr_generate) ||
>> ----------------
>> silvas wrote:
>>> This is definitely not the right thing to do. Don't hijack -Xclang (which is a completely generic thing).
>> Why do we need special handling here ? will the existing behavior (simple forwarding) work?
>
> The original patch does the simple forwarding. In that case, we handle
> the cc1 options (choosing b/w IR or clang instrumentation) in
> CompilerInvocation.cpp. Sean and Chad think this logic should be
> driver.
>
>>
>> Note that intention of the cc1 option is to hide it from the user but make it used by the developers -- so we can make assumption that this option is used only when -fprofile-instr-generate[=...] is specified. You can add diagnostics later during cc1 option processing if it is not the case.
>>
>> Also think about making it easier to flip the default behavior in the future, it might be better to make the cc1 option look like this:
>>
>> -fprofile-instrumentor=[clang|LLVM]
>>
>> if the option is missing, the current default will be 'clang'. In the future just switch it to 'llvm'.
>>
>> ================
>> Comment at: lib/Frontend/CompilerInvocation.cpp:483
>> @@ +482,3 @@
>> +    Opts.ProfileIRInstr = true;
>> +  else
>> +    // Opts.ClangProfInstrGen will be used for Clang instrumentation only.
>> ----------------
>> silvas wrote:
>>> This still isn't factored right. I think at this point the meaning of the driver-level options is not really useful at CC1 level (too convoluted) for it to be useful to pass them through.
>>>
>>> The right thing to do here is probably more like:
>>> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably rename ClangProfInstrGen and ProfileIRInstr to be consistent with each other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
>>> - add logic in Driver to convert from the driver-level options to the CC1 options.
>> It is already pretty close to your proposal -- the only missing thing is cc1 option for ClangProfInstrGen. However given that IR and Clang InstrGen are mutually exclusive, is an additional CC1 option needed?
>>
>>
>> http://reviews.llvm.org/D15829
>>
>>
>>