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

[Sean Silva via cfe-commits]

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/149abb04/attachment.html>