[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:1253
+    Flags<[NoXarchOption, CC1Option]>, Group<f_Group>,
+    HelpText<"Use profi to infer block and edge counts.">;
 def fno_profile_sample_accurate : Flag<["-"], "fno-profile-sample-accurate">,
----------------
A user who reads the help message might reasonably ask "what's profi". Can you please add some documentation about it? It would also be nice with a DocBrief (see e.g. -fprofile-sample-accurate above).


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5763
 
+  // Enable profi in frontend and forward to LLVM invocation
+  if (Args.hasArg(options::OPT_fsample_profile_use_profi)) {
----------------
Perhaps consider skipping this comment, the code is clear enough.


================
Comment at: clang/test/CodeGen/pgo-sample-use-profi.cpp:1
+//Test if profi flat is enabled in frontend as user-facing feature.
+//
----------------
HaoyuZhang wrote:
> MaskRay wrote:
> > Remove the test.
> > 
> > This change only needs a clang/test/Driver test showing that the option passes `-sample-profile-use-profi` to -cc1.
> Thank you for the comments. So we only need to RUN the command for example:
> 
> `//RUN: %clang_cc1 %s -fsample-profile-use-profi -fdebug-pass-manager -emit-llvm -o - 2>&1 | FileCheck %s`
> 
> `//CHECK: Running pass: VerifierPass`
> 
> and without any test code.
> 
I think what MaskRay is saying is that you only need a test in clang/test/Driver/ that does "%clang -c -fsample-profile-use-profi -###" and FileCheck's that the cc1 invocation contains the right -mllvm flag.

(I assume you have tests for the actual functionality somewhere in llvm/test/)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136846