[llvm-profgen] Support a threshold to control hiding warning summaries

[wlei]

Summary

Some small value summaries of warning are less important, add a threshold to control hiding them.

[hoy]

llvm/tools/llvm-profgen/ErrorHandling.h

54

Use <= ?

llvm/tools/llvm-profgen/PerfReader.cpp

46 ↗

(On Diff #428150)

nit: use unsigned int

Wondering what's a good place to do the validation. It should be within [1,100].

[wenlei]

I'm not sure if a universal threshold for filtering warning is a good way to deal with this. Maybe look at this from a different angle -- for some warnings, any instance is bad, for others, we can tolerate them. Hence, an alternative: for critical ones, we should always print out any non-zero value; for others, print them only under a "verbose" mode?

[hoy]

In D125255#3502157, @wenlei wrote:

I'm not sure if a universal threshold for filtering warning is a good way to deal with this. Maybe look at this from a different angle -- for some warnings, any instance is bad, for others, we can tolerate them. Hence, an alternative: for critical ones, we should always print out any non-zero value; for others, print them only under a "verbose" mode?

That's fair. It'd be good to treat warnings of different importance differently.

[wlei]

Updating D125255: [llvm-profgen] Support a threshold to control hiding warning summaries

[wlei]

In D125255#3502157, @wenlei wrote:

I'm not sure if a universal threshold for filtering warning is a good way to deal with this. Maybe look at this from a different angle -- for some warnings, any instance is bad, for others, we can tolerate them. Hence, an alternative: for critical ones, we should always print out any non-zero value; for others, print them only under a "verbose" mode?

Sounds good.

1. add verbose mode to enable to print all of them.
2. add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.
3. IMP, warning of "inconsistent name from symbol table and dwarf info" should be critical. changed thres to 0.

llvm/tools/llvm-profgen/ErrorHandling.h

54

fixed!

llvm/tools/llvm-profgen/PerfReader.cpp

46 ↗

(On Diff #428150)

Added validation in validateCommandLine, it can be 0 to print all the number between [0,1].

[wenlei]

add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.

Do we even need a threshold for emitting critical ones? If it's critical, we should always emit them?

[wlei]

In D125255#3502398, @wenlei wrote:

add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.

Do we even need a threshold for emitting critical ones? If it's critical, we should always emit them?

Sorry for the confusing, " customize value for critical warning" is wrong, I meant the parameter is used to treat warnings of different importance differently. For critical warning, the threshold is 0, then it's always emitted. For non-critical ones, developer can still set a threshold or use the default one, then emit it if it's above it.

[wenlei]

In D125255#3502490, @wlei wrote:

In D125255#3502398, @wenlei wrote:

add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.

Do we even need a threshold for emitting critical ones? If it's critical, we should always emit them?

Sorry for the confusing, " customize value for critical warning" is wrong, I meant the parameter is used to treat warnings of different importance differently. For critical warning, the threshold is 0, then it's always emitted. For non-critical ones, developer can still set a threshold or use the default one, then emit it if it's above it.

Ok, maybe a verbose mode is enough for enabling all warnings. I can see that a threshold for non-critical warnings could be potentially useful, but also wondering if we could just keep things very simple here. The goal is not to give user a lot of control on what to print out, but to make sure 1) the default warning printing is meaningful to average users, 2) advanced users have a way to print everything.

[wlei]

Updating D125255: [llvm-profgen] Support a threshold to control hiding warning summaries

[wlei]

In D125255#3502506, @wenlei wrote:

In D125255#3502490, @wlei wrote:

In D125255#3502398, @wenlei wrote:

add a parameter to emitWarningSummary to customize value for critical warning, default value increase to 20%.

Do we even need a threshold for emitting critical ones? If it's critical, we should always emit them?

Sorry for the confusing, " customize value for critical warning" is wrong, I meant the parameter is used to treat warnings of different importance differently. For critical warning, the threshold is 0, then it's always emitted. For non-critical ones, developer can still set a threshold or use the default one, then emit it if it's above it.

Ok, maybe a verbose mode is enough for enabling all warnings. I can see that a threshold for non-critical warnings could be potentially useful, but also wondering if we could just keep things very simple here. The goal is not to give user a lot of control on what to print out, but to make sure 1) the default warning printing is meaningful to average users, 2) advanced users have a way to print everything.

Sounds good to make things simple, thanks!

[hoy]

LGTM with a minor suggestion.

llvm/tools/llvm-profgen/ErrorHandling.h

53

nit: IsCritical -> AlwaysEmit?

[wlei]

IsCritical -> AlwaysEmit

[wlei]

Updating D125255: [llvm-profgen] Support a threshold to control hiding warning summaries

[wenlei]

I think we also need to decide what should always be emitted. Now there's only one warning that is always emitted. I think we can benefit from another always emit warning that can tells us when profile/binary mismatch and/or the issues like the skylake PMU bug happens. Do we get any indicator now for these issues?

When simplifying, we still want to let user know very meaningful signal, but we don't want to present 10 signals to users, instead we want to give them a couple important signals, I think signal for profile/binary mismatch etc can be considered important?

It looks like we already have a verbose mode show-detailed-warning, maybe we can reuse that?

llvm/tools/llvm-profgen/ErrorHandling.h

 //===-- ErrorHandling.h - Error handler -------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_PROFGEN_ERRORHANDLING_H
 #define LLVM_TOOLS_LLVM_PROFGEN_ERRORHANDLING_H
 
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/WithColor.h"
 #include <system_error>
 
 using namespace llvm;
 
+extern cl::opt<bool> VerboseMode;
+
 [[noreturn]] inline void exitWithError(const Twine &Message,
                                        StringRef Whence = StringRef(),
                                        StringRef Hint = StringRef()) {
   WithColor::error(errs(), "llvm-profgen");
   if (!Whence.empty())
     errs() << Whence.str() << ": ";
   errs() << Message << "\n";
   if (!Hint.empty())

 
 template <typename T, typename... Ts>
 T unwrapOrError(Expected<T> EO, Ts &&... Args) {
   if (EO)
     return std::move(*EO);
   exitWithError(EO.takeError(), std::forward<Ts>(Args)...);
 }
 
-inline void emitWarningSummary(uint64_t Num, uint64_t Total, StringRef Msg) {
-  if (!Total || !Num)
-    return;
-  WithColor::warning() << format("%.2f", static_cast<double>(Num) * 100 / Total)
+inline void emitWarningSummary(uint64_t Num, uint64_t Total, StringRef Msg,
+                               bool AlwaysEmit = false) {
+  if (Total && Num && (VerboseMode || AlwaysEmit))
+    WithColor::warning() << format("%.2f",
+                                   static_cast<double>(Num) * 100 / Total)
                          << "%(" << Num << "/" << Total << ") " << Msg << "\n";
 }
 
 #endif

llvm/tools/llvm-profgen/ProfiledBinary.cpp

if (!hasFuncEntry) {
       if (ShowDetailedWarning)
         WithColor::warning()
             << "Failed to determine function entry for " << F.first
             << " due to inconsistent name from symbol table and dwarf info.\n";
     }
   }
   emitWarningSummary(NoFuncEntryNum, BinaryFunctions.size(),
                      "of functions failed to determine function entry due to "
-                     "inconsistent name from symbol table and dwarf info.");
+                     "inconsistent name from symbol table and dwarf info.",
+                     true);
 }
 
 void ProfiledBinary::load() {
   // Attempt to open the binary.
   OwningBinary<Binary> OBinary = unwrapOrError(createBinary(Path), Path);
   Binary &ExeBinary = *OBinary.getBinary();
 
   auto *Obj = dyn_cast<ELFObjectFileBase>(&ExeBinary);

llvm/tools/llvm-profgen/llvm-profgen.cpp

ProcessId("pid", cl::value_desc("process Id"), cl::ZeroOrMore, cl::init(0),
               cl::cat(ProfGenCategory));
 
 static cl::opt<std::string> DebugBinPath(
     "debug-binary", cl::value_desc("debug-binary"), cl::ZeroOrMore,
     cl::desc("Path of debug info binary, llvm-profgen will load the DWARF info "
              "from it instead of the executable binary."),
     cl::cat(ProfGenCategory));
 
+cl::opt<bool>
+    VerboseMode("verbose", cl::init(false), cl::ZeroOrMore,
+                cl::desc("Enable verbose mode, i.e. print out all warnings."));
+
+static cl::alias VA("v", cl::desc("Alias for --verbose"),
+                    cl::aliasopt(VerboseMode));
+
 extern cl::opt<bool> ShowDisassemblyOnly;
 extern cl::opt<bool> ShowSourceLocations;
 extern cl::opt<bool> SkipSymbolization;
 
 using namespace llvm;
 using namespace sampleprof;
 
 // Validate the command line input.