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
