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

Fri Jul 8 10:22:29 PDT 2016

On Sun, Jul 3, 2016 at 1:50 PM, Sean Silva <chisophugis at gmail.com> wrote:

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

I like this suggestion (the reduced version).

>
> 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."
>
>
I am fine with using source/optimizer. I was mainly against overloading
-fpgo-train to do fine grain control.

David

>
> -- 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/20160708/68cf34dd/attachment.html>