[PATCH] D21823: [Driver] Add flags for enabling both types of PGO Instrumentation

[Xinliang David Li via cfe-commits]

Sanitizers are different IMO. Different santizers are rather independent,
and there is no such thing as -fsantize to mean -fsantize=all

For PGO, in most of the cases, users do not need to care about the
sub-options implied -- by default they should just get the best profiling
(whatever that is). Fine-grained control is more useful for savvy users.
Besides, any flavor of value profiling usually does not make sense to be
used standalone and need to be used with edge profiling for max benefit
(also not supported in our implementation) - so I don't see why pgo control
needs to be done in a way similar to sanitizers.

 In other words, I don't think the primary option -fpgo-train should be
used for fine-grain suboption control.   If we have a different option to
do fine-grain control for PGO, using sanitize like syntax is fine with me:

-fpgo-xxx=y:z  turn on y and z for pgo
-fno-pgo-xxx=y:z  turn off y and z for pgo
or
-fpgo-xxx=no-w:no-y:z   turn on z but turn off w, and y


David


On Sat, Jul 2, 2016 at 6:45 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Sat, Jul 2, 2016 at 1:57 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>
>>
>>
>> On Fri, Jul 1, 2016 at 4:32 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>>> silvas added inline comments.
>>>
>>> ================
>>> Comment at: lib/Driver/Tools.cpp:3560
>>> @@ +3559,3 @@
>>> +    if (PGOTrainArg->getOption().matches(options::OPT_fpgo_train_EQ)) {
>>> +      if (StringRef(PGOTrainArg->getValue()) == "source-cfg")
>>> +        CmdArgs.push_back("-fprofile-instrument=clang");
>>> ----------------
>>> davidxl wrote:
>>> > I think it is better to make the selector  'source' vs 'cfg'.
>>> >
>>> > -fpgo-train=source
>>> > -fpgo-train=cfg
>>> So would `-fpgo-train=cfg` enable icall instrumentation or not?
>>>
>>> My thinking is that down the road we will have one flag for each
>>> independent instrumentation capability (and of course some are mutually
>>> incompatible). This mirrors what the sanitizers do. For example, we would
>>> have:
>>> `-fpgo-train=optimizer-cfg` --> IR edge profiling
>>> `-fpgo-train=optimizer-icall` --> IR icall value profiling
>>> `-fpgo-train=optimizer-...` --> other independent instrumentation we can
>>> do in IR instrumentation.
>>> `-fpgo-train=source-cfg` --> FE edge profiling
>>> `-fpgo-train=source-icall` --> FE icall profiling (if that even exists;
>>> I see some code but there is no user-visible flag)
>>> `-fpgo-train=source-...` --> other FE instrumentation.
>>>
>>> We can then have `-fpgo-train=optimizer` enable e.g.
>>> `-fpgo-train=optimizer-cfg,optimizer-icall`.
>>> We can also have `-fpgo-train=source` enable e.g.
>>> `-fpgo-train=source-cfg`.
>>>
>>> Since instrumentation can have different overheads or different runtime
>>> requirements, users may want to disable some instrumentation.
>>>
>>>
>>
>> -fpgo-train is the new umbrella option that turn on at least edge
>> profiling and some other flavor of value profiling (icall, or stringop, etc
>> in the the future). There is a default setting for source or cfg based PGO.
>> The fine grain control of each sub-option should be done via a different
>> flag -- otherwise we will have to introduce 2 x N sub-options if used with
>> -fpgo-train.  In other words, -fpgo-train=cfg turns on edge and icall
>> profiling as of today.
>>
>> To summarize, I think the following behavior will be nice to users:
>>
>> 1) -fpgo-train when used without any option, it defaults to IR pgo (since
>> it is new option)
>> 2) -fpgo-train=cfg (or some other name) turns on IR pgo
>> 3) -fpgo-train=source turns on FE pgo
>>
>> 4) -fpgo-control=edge:icall to turn on edge profiling and icall
>> profiling. Edge will be turn on by default always, so it is redundant to
>> specify. To turn icall off:
>>     -fpgo-control=^icall
>>
>
> We have a precedent from the sanitizers that I think we should stick to
> e.g. `-fsanitize=undefined -fno-sanitize=vptr`.
> http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation
> http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
>
> I would like to avoid introducing a different syntax (such as the one you
> are proposing using `^` for negation).
>
> -- Sean Silva
>
>
>>
>> 5) -fpgo-train-file=<path> to specify path of raw profile
>>
>> I am not sure if we want -fpgo-apply= as an alias to
>> -fprofile-instr-use=, but not object to it.
>>
>> 6) I also like the capability to combine 1) and 5) by using tags to
>> disambiguate path and cfg|source, but that can be done later.
>>
>> thanks,
>>
>> David
>>
>>
>>
>>
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> http://reviews.llvm.org/D21823
>>>
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160702/3ee75c2a/attachment.html>