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