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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to