[PATCH] D136846: [Driver] Add -fsample-profile-use-profi

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:1254
+    HelpText<"Use profi to infer block and edge counts.">,
+    DocBrief<[{Profi - a flow-based profile inference algorithm is an extended
+               and significantly re-engineered classic MCMF (min-cost max-flow)
----------------
HaoyuZhang wrote:
> hans wrote:
> > I have to say, just reading this text I don't understand what it does.
> > 
> > I think a good description would start with "Infer block and edge counts " and then some kind of summary of how it does that.
> > 
> > I assume profile info is still needed for this (that's the input, right?) That should probably also be explained, and maybe we should warn when using -fsample-profile-use-profi without -fprofile-sample-use?
> > 
> > 
> > My main concern is that there's no documentation for this. How is a user supposed to learn about this feature and how it works? Why can't someone add something to https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization ? Once that is figured out, describing what the option does will probably be easy.
> Sorry for the unclear description of the DocBrief and I have do some modification. 
> 
> A checking has been added for ensuring that -fsample-profile-use-profi is only allowed with fprofile-sample-use. Otherwise, there will be an error.
> 
> About the document in above link, do you want me to add some contents about using profi after the patch or invite the author of profi?
It would ideally be added in this patch. Inviting the author of profi (I didn't realize it was a different person) sounds like a good idea.


================
Comment at: clang/include/clang/Driver/Options.td:1257
+               basic block counts to branch probabilites to fix them by extended
+               and re-engineered classic MCMF (min-cost max-flow) approach.}]>;
 def fno_profile_sample_accurate : Flag<["-"], "fno-profile-sample-accurate">,
----------------
Thanks, this is much better!

(nit: s/converting/convert/)


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5763
 
+  if (Args.hasArg(options::OPT_fsample_profile_use_profi)) {
+    if (Args.hasArg(options::OPT_fprofile_sample_use_EQ)) {
----------------
Could you use `tools::getLastProfileSampleUseArg` instead? It covers all the spellings of the option.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5768
+    } else {
+      D.Diag(diag::err_drv_argument_only_allowed_with)
+          << "-fsample-profile-use-profi"
----------------
I think if you did instead:

```
if (getLastProfileSampleUseArg(Args) && Args.hasArg(options::OPT_fsample_profile_use_profi)) {
  CmdArgs.push_back(...
}
```

then clang would warn about the option being unused if profile use is not enabled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136846/new/

https://reviews.llvm.org/D136846