<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 2, 2016 at 7:38 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">Sanitizers are different IMO. Different santizers are rather independent, and there is no such thing as -fsantize to mean -fsantize=all<div><br></div><div>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. </div></div></blockquote><div><br></div><div>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>`.</div><div><br></div><div>While -fpgo-apply= is technically redundant with -fprofile-instr-use, I think it is useful for consistency.</div><div><br></div><div>I'm also indifferent to adding `-fpgo-train=source`/`-fpgo-train=optimizer` in this patch.</div><div><br></div><div>btw, I don't understand the intuition for calling IRPGO "cfg"; maybe you could explain?</div><div>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."</div><div><br></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><br></div><div> 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:</div><div><br></div><div>-fpgo-xxx=y:z  turn on y and z for pgo</div><div>-fno-pgo-xxx=y:z  turn off y and z for pgo</div><div>or</div><div>-fpgo-xxx=no-w:no-y:z   turn on z but turn off w, and y</div><span><font color="#888888"><div><br></div><div><br></div><div>David</div><div><br></div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 2, 2016 at 6:45 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"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>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>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></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" target="_blank">http://clang.llvm.org/docs/UsersManual.html#controlling-code-generation</a></div><div><a href="http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html" target="_blank">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><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span><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><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></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>