Add -fdiagnostics-show-hotness

[anemet]

Summary

I've recently added the ability for optimization remarks to include the
hotness of the corresponding code region. This uses PGO and allows
filtering of the optimization remarks by relevance. The idea was first
discussed here:
http://thread.gmane.org/gmane.comp.compilers.llvm.devel/98334

The general goal is to produce a YAML file with the remarks. Then, an
external tool could dynamically filter these by hotness and perhaps by
other things.

That said it makes sense to also expose this at the more basic level
where we just include the hotness info with each optimization remark.
For example, in D22694, the clang flag was pretty useful to measure the
overhead of the additional analyses required to include hotness.
(Without the flag we don't even run the analyses.)

For the record, Hal has already expressed support for the idea of this
patch on IRC.

[aaron.ballman]

include/clang/Basic/DiagnosticDriverKinds.td

186

Given that there's only one driver option that requires this diagnostic, I would replace the %0 and %1 directives with their hard-coded text for right now (unless you plan on needing this generality in the near future). If we need to generalize the diagnostic in the future, we can do so using a %select directive.

[anemet]

Address Aaron's comment

[anemet]

Add more reviewers suggested by Aaron. @dnovillo, @rsmith, is my thinking correct that this new flag does not belong to the R_Group.

[anemet]

Since -Rpass-with-hotness is not part of R_group, we need to manually forward
it to clang -cc1. I've also extended the test to cover this bug.

[aaron.ballman]

This LGTM, but you should wait for confirmation on the behavior of the R_Group behavior from @dnovillo or @rsmith before committing.

[anemet]

@dnovillo or @rsmith, can you please confirm if you agree that this new option -Rpass-with-hotness should not be part of R_group. R_group options enable/disable groups of remarks whereas this one is only modifying the text of the remarks. Thanks!

[dnovillo]

In D23284#526389, @anemet wrote:

@dnovillo or @rsmith, can you please confirm if you agree that this new option -Rpass-with-hotness should not be part of R_group. R_group options enable/disable groups of remarks whereas this one is only modifying the text of the remarks. Thanks!

I'm fine with it, but I don't have much of a say in how option groups are organized. If Richard agrees, then it LGTM.

[anemet]

In D23284#526413, @dnovillo wrote:

I'm fine with it, but I don't have much of a say in how option groups are organized. If Richard agrees, then it LGTM.

Ping. Would it be OK to commit this with the two LGTMs? I am around to fix any fall-outs or post-commit reviews from Richard.

[rsmith]

Sorry I missed this until now.

I think this should not be an -R option at all; like -W flags, the idea is for -R to only act as a filter for which diagnostics are shown. This option seems much more closely related to options like -fdiagnostics-show-option and -fdiagnostics-format=, and so should probably have a -fdiagnostics-something name.

[anemet]

In D23284#534879, @rsmith wrote:

I think this should not be an -R option at all; like -W flags, the idea is for -R to only act as a filter for which diagnostics are shown. This option seems much more closely related to options like -fdiagnostics-show-option and -fdiagnostics-format=, and so should probably have a -fdiagnostics-something name.

Sounds fine to me. Any preference from:

-fdiagnostics-include-hotness-in-remarks
-fpass-diagnostics-with-hotness
-fpass-diagnostics-include-hotness

or something else?

[rsmith]

In D23284#534919, @anemet wrote:

In D23284#534879, @rsmith wrote:

I think this should not be an -R option at all; like -W flags, the idea is for -R to only act as a filter for which diagnostics are shown. This option seems much more closely related to options like -fdiagnostics-show-option and -fdiagnostics-format=, and so should probably have a -fdiagnostics-something name.

Sounds fine to me. Any preference from:

-fdiagnostics-include-hotness-in-remarks
-fpass-diagnostics-with-hotness
-fpass-diagnostics-include-hotness

or something else?

I think this should start with -fdiagnostics to group it with other similar flags. -fdiagnostics-include-hotness-in-remarks seems too specific to me -- I could imagine generating warnings from the middle-end including hotness information, not just remarks. How about -fdiagnostics-show-hotness? (This would fit nicely with the existing -fdiagnostics-show-* flags.)

[anemet]

In D23284#535258, @rsmith wrote:

I think this should start with -fdiagnostics to group it with other similar flags. -fdiagnostics-include-hotness-in-remarks seems too specific to me -- I could imagine generating warnings from the middle-end including hotness information, not just remarks. How about -fdiagnostics-show-hotness? (This would fit nicely with the existing -fdiagnostics-show-* flags.)

SGTM. I'll update the patch.

[anemet]

This renames the flag to -fdiagnostics-show-hotness as requested by Richard.
Also added the missing documentation bits.

[rsmith]

lib/Frontend/CompilerInvocation.cpp

823

You should just use hasArg here, there is no -fno-diagnostics-show-hotness flag for -cc1.

[anemet]

Address Richard's comment

include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_preprocessed_input_file_unused : Warning<
   "%0: previously preprocessed input%select{ unused when '%2' is present|}1">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_unused_argument : Warning<
   "argument unused during compilation: '%0'">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_empty_joined_argument : Warning<
   "joined argument expects additional value: '%0'">,
   InGroup<UnusedCommandLineArgument>;
+def warn_drv_argument_requires : Warning<
+  "argument '%0' requires %1">,
+  InGroup<UnusedCommandLineArgument>;
 def warn_drv_clang_unsupported : Warning<
   "the clang compiler does not support '%0'">;
 def warn_drv_deprecated_arg : Warning<
   "argument '%0' is deprecated, use '%1' instead">, InGroup<Deprecated>;
 def warn_drv_assuming_mfloat_abi_is : Warning<
   "unknown platform, assuming -mfloat-abi=%0">;
 def warn_ignoring_ftabstop_value : Warning<
   "ignoring invalid -ftabstop value '%0', using default value %1">;

include/clang/Driver/Options.td

def Rpass_missed_EQ : Joined<["-"], "Rpass-missed=">, Group<R_value_Group>,
   HelpText<"Report missed transformations by optimization passes whose "
            "name matches the given POSIX regular expression">;
 def Rpass_analysis_EQ : Joined<["-"], "Rpass-analysis=">, Group<R_value_Group>,
   Flags<[CC1Option]>,
   HelpText<"Report transformation analysis from optimization passes whose "
            "name matches the given POSIX regular expression">;
 def R_Joined : Joined<["-"], "R">, Group<R_Group>, Flags<[CC1Option, CoreOption]>,
   MetaVarName<"<remark>">, HelpText<"Enable the specified remark">;
+// Don't include in R_Group, since this does not enable any remark group.
+def Rpass_with_hotness : Flag<["-"], "Rpass-with-hotness">, Flags<[CC1Option]>,
+  HelpText<"If profile-guided optimization information is available, "
+           "include hotness of transformed code region in messages "
+           "reported by -Rpass* options">;
+def Rno_pass_with_hotness : Flag<["-"], "Rno-pass-with-hotness">, Flags<[CC1Option]>;
 def S : Flag<["-"], "S">, Flags<[DriverOption,CC1Option]>, Group<Action_Group>,
   HelpText<"Only run preprocess and compilation steps">;
 def Tbss : JoinedOrSeparate<["-"], "Tbss">, Group<T_Group>;
 def Tdata : JoinedOrSeparate<["-"], "Tdata">, Group<T_Group>;
 def Ttext : JoinedOrSeparate<["-"], "Ttext">, Group<T_Group>;
 def T : JoinedOrSeparate<["-"], "T">, Group<T_Group>;
 def U : JoinedOrSeparate<["-"], "U">, Group<CompileOnly_Group>, Flags<[CC1Option]>;
 def V : JoinedOrSeparate<["-"], "V">, Flags<[DriverOption, Unsupported]>;

include/clang/Frontend/CodeGenOptions.def

 
 /// The default TLS model to use.
 ENUM_CODEGENOPT(DefaultTLSModel, TLSModel, 2, GeneralDynamicTLSModel)
 
 /// Number of path components to strip when emitting checks. (0 == full
 /// filename)
 VALUE_CODEGENOPT(EmitCheckPathComponentsToStrip, 32, 0)
 
+/// Whether to report the hotness of the code region for optimization remarks.
+CODEGENOPT(PassRemarksWithHotness, 1, 0)
+
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT
 

lib/CodeGen/CodeGenAction.cpp

void HandleTranslationUnit(ASTContext &C) override {
         Ctx.getInlineAsmDiagnosticHandler();
       void *OldContext = Ctx.getInlineAsmDiagnosticContext();
       Ctx.setInlineAsmDiagnosticHandler(InlineAsmDiagHandler, this);
 
       LLVMContext::DiagnosticHandlerTy OldDiagnosticHandler =
           Ctx.getDiagnosticHandler();
       void *OldDiagnosticContext = Ctx.getDiagnosticContext();
       Ctx.setDiagnosticHandler(DiagnosticHandler, this);
+      Ctx.setDiagnosticHotnessRequested(CodeGenOpts.PassRemarksWithHotness);
 
       // Link LinkModule into this module if present, preserving its validity.
       for (auto &I : LinkModules) {
         unsigned LinkFlags = I.first;
         CurLinkModule = I.second.get();
         if (Linker::linkModules(*getModule(), std::move(I.second), LinkFlags))
           return;
       }
assert(D.getSeverity() == llvm::DS_Remark ||
          D.getSeverity() == llvm::DS_Warning);
 
   StringRef Filename;
   unsigned Line, Column;
   bool BadDebugInfo = false;
   FullSourceLoc Loc = getBestLocationFromDebugLoc(D, BadDebugInfo, Filename,
       Line, Column);
 
+  std::string Msg;
+  raw_string_ostream MsgStream(Msg);
+  MsgStream << D.getMsg().str();
+
+  if (D.getHotness())
+    MsgStream << " (hotness: " << *D.getHotness() << ")";
+
   Diags.Report(Loc, DiagID)
       << AddFlagValue(D.getPassName() ? D.getPassName() : "")
-      << D.getMsg().str();
+      << MsgStream.str();
 
   if (BadDebugInfo)
     // If we were not able to translate the file:line:col information
     // back to a SourceLocation, at least emit a note stating that
     // we could not translate this location. This can happen in the
     // case of #line directives.
     Diags.Report(Loc, diag::note_fe_backend_invalid_loc)
         << Filename << Line << Column;

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   }
 
   if (Arg *A = Args.getLastArg(OPT_Rpass_analysis_EQ)) {
     Opts.OptimizationRemarkAnalysisPattern =
         GenerateOptimizationRemarkRegex(Diags, Args, A);
     NeedLocTracking = true;
   }
 
+  Opts.PassRemarksWithHotness =
+      Args.hasFlag(options::OPT_Rpass_with_hotness,
+                   options::OPT_Rno_pass_with_hotness, /*default*/ false);
+  if (Opts.PassRemarksWithHotness &&
+      Opts.getProfileUse() == CodeGenOptions::ProfileNone)
+    Diags.Report(diag::warn_drv_argument_requires) << "-Rpass-with-hotness"
+                                                   << "profile information";
+
   // If the user requested to use a sample profile for PGO, then the
   // backend will need to track source location information so the profile
   // can be incorporated into the IR.
   if (!Opts.SampleProfileFile.empty())
     NeedLocTracking = true;
 
   // If the user requested a flag that requires source locations available in
   // the backend, make sure that the backend tracks source location information.

test/Frontend/Inputs/optimization-remark-with-hotness.proftext

+foo
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+30
+
+bar
+# Func Hash:
+0
+# Num Counters:
+1
+# Counter Values:
+30
+
+main
+# Func Hash:
+4
+# Num Counters:
+2
+# Counter Values:
+1
+30
+

test/Frontend/optimization-remark-with-hotness.c

+// RUN: llvm-profdata merge \
+// RUN:     %S/Inputs/optimization-remark-with-hotness.proftext   \
+// RUN:     -o %t.profdata
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name \
+// RUN:     optimization-remark-with-hotness.c %s -emit-llvm-only \
+// RUN:     -fprofile-instrument-use-path=%t.profdata -Rpass=inline \
+// RUN:     -Rpass-analysis=inline -Rpass-with-hotness -verify
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name \
+// RUN:     optimization-remark-with-hotness.c %s -emit-llvm-only \
+// RUN:     -fprofile-instrument-use-path=%t.profdata -Rpass=inline \
+// RUN:     -Rpass-analysis=inline 2>&1 | FileCheck -check-prefix=HOTNESS_OFF %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name \
+// RUN:     optimization-remark-with-hotness.c %s -emit-llvm-only \
+// RUN:     -fprofile-instrument-use-path=%t.profdata -Rpass=inline \
+// RUN:     -Rpass-analysis=inline -Rno-pass-with-hotness  2>&1 | FileCheck \
+// RUN:     -check-prefix=HOTNESS_OFF %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name \
+// RUN:     optimization-remark-with-hotness.c %s -emit-llvm-only \
+// RUN:     -Rpass=inline -Rpass-analysis=inline -Rpass-with-hotness  2>&1 \
+// RUN:     | FileCheck -check-prefix=NO_PGO %s
+
+int foo(int x, int y) __attribute__((always_inline));
+int foo(int x, int y) { return x + y; }
+
+int sum = 0;
+
+void bar(int x) {
+  // HOTNESS_OFF: foo inlined into bar
+  // HOTNESS_OFF-NOT: hotness:
+  // NO_PGO: '-Rpass-with-hotness' requires profile information
+  // expected-remark@+2 {{foo should always be inlined (cost=always) (hotness: 30)}}
+  // expected-remark@+1 {{foo inlined into bar (hotness: 30)}}
+  sum += foo(x, x - 2);
+}
+
+int main(int argc, const char *argv[]) {
+  for (int i = 0; i < 30; i++)
+    // expected-remark@+1 {{bar should never be inlined}}
+    bar(argc);
+  return sum;
+}