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