<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 2, 2016 at 1:57 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Fri, Jul 1, 2016 at 4:32 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">silvas added inline comments.<br>
<br>
================<br>
<span>Comment at: lib/Driver/Tools.cpp:3560<br>
@@ +3559,3 @@<br>
+ if (PGOTrainArg->getOption().matches(options::OPT_fpgo_train_EQ)) {<br>
+ if (StringRef(PGOTrainArg->getValue()) == "source-cfg")<br>
+ CmdArgs.push_back("-fprofile-instrument=clang");<br>
----------------<br>
</span><span>davidxl wrote:<br>
> I think it is better to make the selector 'source' vs 'cfg'.<br>
><br>
> -fpgo-train=source<br>
> -fpgo-train=cfg<br>
</span>So would `-fpgo-train=cfg` enable icall instrumentation or not?<br>
<br>
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:<br>
`-fpgo-train=optimizer-cfg` --> IR edge profiling<br>
`-fpgo-train=optimizer-icall` --> IR icall value profiling<br>
`-fpgo-train=optimizer-...` --> other independent instrumentation we can do in IR instrumentation.<br>
`-fpgo-train=source-cfg` --> FE edge profiling<br>
`-fpgo-train=source-icall` --> FE icall profiling (if that even exists; I see some code but there is no user-visible flag)<br>
`-fpgo-train=source-...` --> other FE instrumentation.<br>
<br>
We can then have `-fpgo-train=optimizer` enable e.g. `-fpgo-train=optimizer-cfg,optimizer-icall`.<br>
We can also have `-fpgo-train=source` enable e.g. `-fpgo-train=source-cfg`.<br>
<br>
Since instrumentation can have different overheads or different runtime requirements, users may want to disable some instrumentation.<br>
<div><div><br></div></div></blockquote><div><br></div><div><br></div></span><div>-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.</div><div><br></div><div>To summarize, I think the following behavior will be nice to users:</div><div><br></div><div>1) -fpgo-train when used without any option, it defaults to IR pgo (since it is new option)</div><div>2) -fpgo-train=cfg (or some other name) turns on IR pgo</div><div>3) -fpgo-train=source turns on FE pgo</div><div><br></div><div>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:</div><div> -fpgo-control=^icall</div></div></div></div></blockquote><div><br></div><div>We have a precedent from the sanitizers that I think we should stick to e.g. `-fsanitize=undefined -fno-sanitize=vptr`.</div><div><a href="http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation">http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation</a></div><div><a href="http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html">http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html</a></div><div><br></div><div>I would like to avoid introducing a different syntax (such as the one you are proposing using `^` for negation).</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>5) -fpgo-train-file=<path> to specify path of raw profile</div><div><br></div><div>I am not sure if we want -fpgo-apply= as an alias to -fprofile-instr-use=, but not object to it.</div><div><br></div><div>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.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><span class=""><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><div>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D21823" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21823</a><br>
<br>
<br>
<br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>