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

[Sean Silva via cfe-commits]

On Sat, Jul 2, 2016 at 7:38 PM, Xinliang David Li <davidxl at google.com>
wrote:

> 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.
>

Thanks for the explanation. So I think we can start by reducing this patch
to just `-fpgo-train` (enables IRPGO), `-fpgo-train-file=<path>`, and
`-fpgo-apply=<profdata>`.

While -fpgo-apply= is technically redundant with -fprofile-instr-use, I
think it is useful for consistency.

I'm also indifferent to adding `-fpgo-train=source`/`-fpgo-train=optimizer`
in this patch.

btw, I don't understand the intuition for calling IRPGO "cfg"; maybe you
could explain?
I like "optimizer" because it is easy to document to users, as users
generally understand the difference between the "optimizer" and
source-level analysis of their program. For example, we could document:
"-fpgo-train=source instruments at source-level similar to code coverage
instrumentation. -fpgo-train=optimizer applies the instrumentation inside
the optimizer and has freedom to do sophisticated analyses and
transformations as part of the instrumentation process; these analyses and
transformations allow it to reduce instrumentation overhead and increase
profile accuracy."


-- Sean Silva


>
>  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/20160703/afcddd99/attachment.html>