On Fri, Jul 8, 2016 at 10:22 AM, Xinliang David Li <davi...@google.com>
wrote:
>
>
> On Sun, Jul 3, 2016 at 1:50 PM, Sean Silva <chisophu...@gmail.com> wrote:
>
>>
>>
>> On Sat, Jul 2, 2016 at 7:38 PM, Xinliang David Li <davi...@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.
>
Ok, thanks for the clarification.
-- Sean Silva
>
> 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 <chisophu...@gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Sat, Jul 2, 2016 at 1:57 PM, Xinliang David Li <davi...@google.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Fri, Jul 1, 2016 at 4:32 PM, Sean Silva <chisophu...@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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
