[remark][diagnostics] Using clang diagnostic handler for IR input files

[xur]

Summary

For IR input files, we currently use LLVM diagnostic handler even the
compilation is from clang. As a result, we are not able to use -Rpass
to get the transformation reports. Some warnings are not handled
properly either: We found many mysterious warnings in our ThinLTO backend
compilations in SamplePGO and CSPGO. An example of the warning:

warning: net/proto2/public/metadata_lite.h:51:21: 0.02% (1 / 4999)

This turns out to be a warning by Wmisexpect, which is supposed to be
filtered out by default. But since the filter is in clang's
diagnostic hander, we emit these incomplete warnings from LLVM's
diagnostic handler.

This patch uses clang diagnostic handler for IR input files. We create
a fake backendconsumer just to install the diagnostic handler.
The location information is tightly coupled with the sourcemanager which
is not available in IR input files. I just prepend the location from DebugLoc
to the warning/remark messages. I think this is much simpler way than
to pass DebugLoc to DiagnosticEngine (I tried this approach, but it seemed to
require much more extensive changes).

With this change, we will have proper handling of all the warnings and we can
use -Rpass* options in IR input files compilation.
Also note that with is patch, LLVM's diagnostic options, like
"-mllvm -pass-remarks=*", are no longer be able to get optimization remarks.

[tejohnson]

Thanks for fixing this missing -Rpass support!

clang/lib/CodeGen/CodeGenAction.cpp

598

Does this only happen with IR input? Does it always happen with IR input? If not, what happens when we have a non-null Context but IR input? It sounds like it should still always return an invalid Loc since there is no SourceManager? In that case can you set a flag on the BackendConsumer so we can bail out directly in the IR input case, instead of going through the motions only to get an invalid Loc back anyway? It would also be more clear.

649

Can you add a test for this one?

Also, what determines the format of the message when Loc is valid? I was trying to figure out where that got determined, but couldn't easily track it down.

clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c

9

In this case (since no -Wmisexpect), presumably we should not be getting the warning, whereas without your fix we were, correct? In that case, please add a NOT check to confirm that we don't get the message anymore.

[xur]

clang/lib/CodeGen/CodeGenAction.cpp

598

Before this patch, IR input does not called to this function. IR input has a null Context (so it will seg-fault at the dereference at line 598). Context == nullptr should be only happening with the empty BackendConsume that installed in this patch.
I tried to set Context but the whole point was to get SrouceManager which was not available for IR input.

649

Do you mean adding a test to test the path of branch starting line 650?
(For the case of valid Loc, I don't think we need to test as it's not changed.)

If so, there is already a test for this:
clang/test/CodeGen/backend-unsupported-error.ll
I think this calls to llvm's diagnostic handler for IR input files (before this patch). The format is here:
DiagnosticInfoUnsupported::print(...) in llvm/lib/IR/DiagnosticInfo.cpp

For the case of input other than IR files, the Diags should go to clang diagnostic handler. This patch might change the behavior.

I think I will check Context == nullptr directly (rather Loc.isValid). This way we don't need to call into getBestLocationFromDebugLoc(), and it will not affect non-IR input files.

clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c

9

Yes. You are right.
I will add a NOT check to confirm this.

[xur]

Integrated Teresa's review comments.

[tejohnson]

clang/lib/CodeGen/CodeGenAction.cpp

598

Since Context==nullptr iff IR input, then for clarity can you add a comment by these checks that this case is specifically corresponding to when we have IR input?

(Also, I was a little confused when I first reviewed this since I didn't realize this Context variable was an ASTContext and not the LLVMContext being passed in to the new BackendConsumer constructor, now I see why it is always null in that case.)

649

Do you mean adding a test to test the path of branch starting line 650?

Yes, but:

If so, there is already a test for this:
clang/test/CodeGen/backend-unsupported-error.ll

I see, I didn't realize this was already being tested with IR input. Nevermind then.

The format is here:
DiagnosticInfoUnsupported::print(...) in llvm/lib/IR/DiagnosticInfo.cpp

Ok thanks. Looking at that file, I'm wondering if we can use those facilities directly rather than cloning the formatting code here. I.e. instead of constructing a MsgStream manually here, duplicating the code in DiagnosticInfoUnsupported::print, couldn't this handling be something like:

DiagnosticPrinterRawOStream DP(MsgStream);
D.print(DP);

These lines would subsume the manual setup of MsgStream in the null Context case, as well as the MsgStream << D.getMessage() line below it. Then you would still call Diags.Report with MsgStream.str() as usual.

And ditto for the other 2 types of DiagnosticInfos below. This mechanism is used in quite a few diagnostic handlers setup in LLVM (e.g. the one in gold-plugin.cpp).

[xur]

Thanks Teresa for the suggestion: using the print() function in the llvm diagnosticPrinter is a lot better than duplicating the code. I changed the patch to use print() function. Also added comment as suggested by Teresa.

[tejohnson]

LGTM. One more request for a comment below that I forgot to add earlier.

clang/lib/CodeGen/CodeGenAction.cpp

154

Add comment as to what specific case this new constructor is for.

[jeroen.dobbelaere]

This change triggers a crash in a release build of llvm:
See https://bugs.llvm.org/show_bug.cgi?id=44896

clang/lib/CodeGen/CodeGenAction.cpp

BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
           LLVMIRGeneration("irgen", "LLVM IR Generation Time"),
           LLVMIRGenerationRefCount(0),
           Gen(CreateLLVMCodeGen(Diags, InFile, HeaderSearchOpts, PPOpts,
                                 CodeGenOpts, C, CoverageInfo)),
           LinkModules(std::move(LinkModules)) {
       FrontendTimesIsEnabled = TimePasses;
       llvm::TimePassesIsEnabled = TimePasses;
     }
+
+    // This constructor is used in installing an empty BackendConsumer
+    // to use the clang diagnostic handler for IR input files. It avoids
+    // initializing the OS field.
+    BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
+                    const HeaderSearchOptions &HeaderSearchOpts,
+                    const PreprocessorOptions &PPOpts,
+                    const CodeGenOptions &CodeGenOpts,
+                    const TargetOptions &TargetOpts,
+                    const LangOptions &LangOpts, bool TimePasses,
+                    SmallVector<LinkModule, 4> LinkModules, LLVMContext &C,
+                    CoverageSourceInfo *CoverageInfo = nullptr)
+        : Diags(Diags), Action(Action), HeaderSearchOpts(HeaderSearchOpts),
+          CodeGenOpts(CodeGenOpts), TargetOpts(TargetOpts), LangOpts(LangOpts),
+          Context(nullptr),
+          LLVMIRGeneration("irgen", "LLVM IR Generation Time"),
+          LLVMIRGenerationRefCount(0),
+          Gen(CreateLLVMCodeGen(Diags, "", HeaderSearchOpts, PPOpts,
+                                CodeGenOpts, C, CoverageInfo)),
+          LinkModules(std::move(LinkModules)) {
+      FrontendTimesIsEnabled = TimePasses;
+      llvm::TimePassesIsEnabled = TimePasses;
+    }
     llvm::Module *getModule() const { return Gen->GetModule(); }
     std::unique_ptr<llvm::Module> takeModule() {
       return std::unique_ptr<llvm::Module>(Gen->ReleaseModule());
     }
 
     CodeGenerator *getCodeGenerator() { return Gen.get(); }
 
     void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
BackendConsumer::StackSizeDiagHandler(const llvm::DiagnosticInfoStackSize &D) {
   return false;
 }
 
 const FullSourceLoc BackendConsumer::getBestLocationFromDebugLoc(
     const llvm::DiagnosticInfoWithLocationBase &D, bool &BadDebugInfo,
     StringRef &Filename, unsigned &Line, unsigned &Column) const {
   SourceManager &SourceMgr = Context->getSourceManager();
   FileManager &FileMgr = SourceMgr.getFileManager();
   SourceLocation DILoc;
 
   if (D.isLocationAvailable()) {
     D.getLocation(Filename, Line, Column);
     if (Line > 0) {
       auto FE = FileMgr.getFile(Filename);
       if (!FE)
         FE = FileMgr.getFile(D.getAbsolutePath());
       if (FE) {

 void BackendConsumer::UnsupportedDiagHandler(
     const llvm::DiagnosticInfoUnsupported &D) {
   // We only support errors.
   assert(D.getSeverity() == llvm::DS_Error);
 
   StringRef Filename;
   unsigned Line, Column;
   bool BadDebugInfo = false;
-  FullSourceLoc Loc =
-      getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
+  FullSourceLoc Loc;
+  std::string Msg;
+  raw_string_ostream MsgStream(Msg);
 
-  Diags.Report(Loc, diag::err_fe_backend_unsupported) << D.getMessage().str();
+  // Context will be nullptr for IR input files, we will construct the diag
+  // message from llvm::DiagnosticInfoUnsupported.
+  if (Context != nullptr) {
+    Loc = getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
+    MsgStream << D.getMessage();
+  } else {
+    DiagnosticPrinterRawOStream DP(MsgStream);
+    D.print(DP);
+  }
+  Diags.Report(Loc, diag::err_fe_backend_unsupported) << 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;
 }
 
 void BackendConsumer::MisExpectDiagHandler(
     const llvm::DiagnosticInfoMisExpect &D) {
   StringRef Filename;
   unsigned Line, Column;
   bool BadDebugInfo = false;
-  FullSourceLoc Loc =
-      getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
+  FullSourceLoc Loc;
+  std::string Msg;
+  raw_string_ostream MsgStream(Msg);
+  DiagnosticPrinterRawOStream DP(MsgStream);
 
-  Diags.Report(Loc, diag::warn_profile_data_misexpect) << D.getMsg().str();
+  // Context will be nullptr for IR input files, we will construct the diag
+  // message from llvm::DiagnosticInfoMisExpect.
+  if (Context != nullptr) {
+    Loc = getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
+    MsgStream << D.getMsg();
+  } else {
+    DiagnosticPrinterRawOStream DP(MsgStream);
+    D.print(DP);
+  }
+  Diags.Report(Loc, diag::warn_profile_data_misexpect) << 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;
 }
 
 void BackendConsumer::EmitOptimizationMessage(
     const llvm::DiagnosticInfoOptimizationBase &D, unsigned DiagID) {
   // We only support warnings and remarks.
   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);
-
+  FullSourceLoc Loc;
   std::string Msg;
   raw_string_ostream MsgStream(Msg);
+
+  // Context will be nullptr for IR input files, we will construct the remark
+  // message from llvm::DiagnosticInfoOptimizationBase.
+  if (Context != nullptr) {
+    Loc = getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
     MsgStream << D.getMsg();
+  } else {
+    DiagnosticPrinterRawOStream DP(MsgStream);
+    D.print(DP);
+  }
 
   if (D.getHotness())
     MsgStream << " (hotness: " << *D.getHotness() << ")";
 
   Diags.Report(Loc, DiagID)
       << AddFlagValue(D.getPassName())
       << MsgStream.str();
 
if (getCurrentFileKind().getLanguage() == Language::LLVM_IR) {
 
     EmbedBitcode(TheModule.get(), CodeGenOpts,
                  MainFile->getMemBufferRef());
 
     LLVMContext &Ctx = TheModule->getContext();
     Ctx.setInlineAsmDiagnosticHandler(BitcodeInlineAsmDiagHandler,
                                       &Diagnostics);
 
+    // Set clang diagnostic handler. To do this we need to create a fake
+    // BackendConsumer.
+    BackendConsumer Result(BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),
+                           CI.getPreprocessorOpts(), CI.getCodeGenOpts(),
+                           CI.getTargetOpts(), CI.getLangOpts(),
+                           CI.getFrontendOpts().ShowTimers,
+                           std::move(LinkModules), *VMContext, nullptr);
+    Ctx.setDiagnosticHandler(
+        std::make_unique<ClangDiagnosticHandler>(CodeGenOpts, &Result));
+
     Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
         setupOptimizationRemarks(
-            Ctx, CodeGenOpts.OptRecordFile,
-            CodeGenOpts.OptRecordPasses,
-            CodeGenOpts.OptRecordFormat,
-            CodeGenOpts.DiagnosticsWithHotness,
+            Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses,
+            CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness,
             CodeGenOpts.DiagnosticsHotnessThreshold);
 
     if (Error E = OptRecordFileOrErr.takeError()) {
       reportOptRecordError(std::move(E), Diagnostics, CodeGenOpts);
       return;
     }
     std::unique_ptr<llvm::ToolOutputFile> OptRecordFile =
         std::move(*OptRecordFileOrErr);
 
-    EmitBackendOutput(Diagnostics, CI.getHeaderSearchOpts(),
-                      CodeGenOpts, TargetOpts, CI.getLangOpts(),
+    EmitBackendOutput(Diagnostics, CI.getHeaderSearchOpts(), CodeGenOpts,
+                      TargetOpts, CI.getLangOpts(),
                       CI.getTarget().getDataLayout(), TheModule.get(), BA,
                       std::move(OS));
 
     if (OptRecordFile)
       OptRecordFile->keep();
     return;
   }
 

clang/test/CodeGen/Inputs/thinlto_expect1.proftext

+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
[phone]
+# Num Counters:
+2
+# Counter Values:
+12
+24
+

clang/test/CodeGen/Inputs/thinlto_expect2.proftext

+# CSIR level Instrumentation Flag
+:csir
+foo
+# Func Hash:
[phone]
+# Num Counters:
+2
+# Counter Values:
+12
+24
+
+foo
+# Func Hash:
+1152921530178146050
+# Num Counters:
+2
+# Counter Values:
+24
+12
+

clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c

+// Test clang diagnositic handler works in IR file compilation.
+
+// REQUIRES: x86-registered-target
+
+// RUN: llvm-profdata merge -o %t1.profdata %S/Inputs/thinlto_expect1.proftext
+// RUN: %clang -O2 -fexperimental-new-pass-manager -flto=thin -g -fprofile-use=%t1.profdata -c -o %t1.bo %s
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck %s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj -Wmisexpect 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -fprofile-instrument-use-path=%t2.profdata -emit-obj 2>&1 | FileCheck %s -allow-empty -check-prefix=CHECK-NOWARNING
+
+int sum;
+__attribute__((noinline)) void bar() {
+  sum = 1234;
+}
+
+__attribute__((noinline)) void foo(int m) {
+  if (__builtin_expect(m > 9, 1))
+    bar();
+}
+// CHECK-REMARK: remark: {{.*}}.c:
+// CHECK-WARNING: warning: Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24) of profiled executions.
+// CHECK-NOWARNING-NOT: warning: {{.*}}.c:{{[0-9]*}}:26: 50.00% (12 / 24)

clang/test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll

 ; YAML-NEXT:   - String:          '(cost='
 ; YAML-NEXT:   - Cost:            '0'
 ; YAML-NEXT:   - String:          ', threshold='
 ; YAML-NEXT:   - Threshold:       '337'
 ; YAML-NEXT:   - String:          ')'
 ; YAML-NEXT: ...
 
 ; Next try with pass remarks to stderr
-; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -mllvm -pass-remarks=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
+; RUN: %clang -target x86_64-scei-ps4 -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -Rpass=inline -fdiagnostics-show-hotness -o %t2.o -c 2>&1 | FileCheck %s
 
 ; CHECK: tinkywinky inlined into main with (cost=0, threshold=337) (hotness: 300)
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-scei-ps4"
 
 declare i32 @patatino()