[Clang] Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled

[vivekvpandya]

Summary

Clang changes related to Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled

[vivekvpandya]

Updated the patch! I don't think now we require to save OldDiagnosticHandler and context as we have ClangDiagnosticHandler class with creation of CreateASTConsumer, but I need some review.

[vivekvpandya]

Clang-formated

[anemet]

Please clean this up as well (don't have commented-out lines) so that it's ready to go with the LLVM patch.

lib/CodeGen/CodeGenAction.cpp

302–305

Remove this.

761

Again don't overload DiagContext as such. Have a dedicated field for the BackendConsumer.

881–883

Any reason you moved where we set this up?

[vivekvpandya]

lib/CodeGen/CodeGenAction.cpp

881–883

1. At older place I was not able to define ClangDiagnosticHandler class as it will require definition of BackendComsumer and vice versa.
2. and this seems to be appropriate place to create and tie DiagnosticHandler to LLVMContext

But I am not sure about why old diagnostic handler was tied back to context that is why I wanted someone from clang to look at this.

[vivekvpandya]

Update

[anemet]

lib/CodeGen/CodeGenAction.cpp

881–883

At older place I was not able to define ClangDiagnosticHandler class as it will require definition of BackendComsumer and vice versa.

Why? That was inside BackendConsumer.

But I am not sure about why old diagnostic handler was tied back to context that is why I wanted someone from clang to look at this.

You can just save the old DiagHandler object instead.

[vivekvpandya]

Why? That was inside BackendConsumer.

I was getting incomplete type error.

You can just save the old DiagHandler object instead.
I don't think now we need to do that as per my understanding CodeGen i.e emitting LLVM IR is last phase in clang which will pass control to LLVMContext. LLVMContext will have Base class of DiagnosticHandler which will not handle diagnostic and return false so LLVMContext::diagnose() will print diagnostic with

DiagnosticPrinterRawOStream DP(errs());

errs() << getDiagnosticMessagePrefix(DI.getSeverity()) << ": ";
DI.print(DP);
errs() << "\n";

and clang's diagnostic handler also does similar thing so if we keep ClangDiagnosticHandler pointer in LLVMContext it should not break.

[anemet]

In D37196#868320, @vivekvpandya wrote:

Why? That was inside BackendConsumer.

I was getting incomplete type error.

You may have to move the class declaration of ClangDiagnosticHandler before the BackendConsumer and move the definition of ClangDiagnosticHandler::handleDiagnositic out of line since that is where you use BackendConsumer. Something like:

class BackendConsumer;

class ClangDiagnosticHandler {
public:
  ...  // as before
  bool handleDiagnostics(const DiagnosticInfo &DI) override;
  ...  
private:
  ... 
}

class BackendConsumer {
  ...
}

bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
  ...
}

You can just save the old DiagHandler object instead.

I don't think now we need to do that as per my understanding CodeGen i.e emitting LLVM IR is last phase in clang which will pass control to LLVMContext. LLVMContext will have Base class of DiagnosticHandler which will not handle diagnostic and return false so LLVMContext::diagnose() will print diagnostic with

DiagnosticPrinterRawOStream DP(errs());

errs() << getDiagnosticMessagePrefix(DI.getSeverity()) << ": ";
DI.print(DP);
errs() << "\n";

and clang's diagnostic handler also does similar thing so if we keep ClangDiagnosticHandler pointer in LLVMContext it should not break.

That would have been true even before and we still restored it, no? I guess I am not sure why we used to restore this so I would just do it regardless and not change functionality.

If you want we can separately take a look whether we need to restore these original handlers.

[vivekvpandya]

update

[anemet]

LGTM.

[shafik]

It looks like these changes are already landed so closing

lib/CodeGen/CodeGenAction.cpp

IRGenFinished = true;
       // Install an inline asm handler so that diagnostics get printed through
       // our diagnostics hooks.
       LLVMContext &Ctx = getModule()->getContext();
       LLVMContext::InlineAsmDiagHandlerTy OldHandler =
         Ctx.getInlineAsmDiagnosticHandler();
       void *OldContext = Ctx.getInlineAsmDiagnosticContext();
       Ctx.setInlineAsmDiagnosticHandler(InlineAsmDiagHandler, this);
 
-      LLVMContext::DiagnosticHandlerTy OldDiagnosticHandler =
-          Ctx.getDiagnosticHandler();
-      void *OldDiagnosticContext = Ctx.getDiagnosticContext();
-      Ctx.setDiagnosticHandler(DiagnosticHandler, this);
+      //LLVMContext::DiagnosticHandlerTy OldDiagnosticHandler =
+      //    Ctx.getDiagnosticHandler();
+     // void *OldDiagnosticContext = Ctx.getDiagnosticContext();
+     // Ctx.setDiagnosticHandler(DiagnosticHandler, this);
       Ctx.setDiagnosticsHotnessRequested(CodeGenOpts.DiagnosticsWithHotness);
       if (CodeGenOpts.DiagnosticsHotnessThreshold != 0)
         Ctx.setDiagnosticsHotnessThreshold(
             CodeGenOpts.DiagnosticsHotnessThreshold);
 
       std::unique_ptr<llvm::tool_output_file> OptRecordFile;
       if (!CodeGenOpts.OptRecordFile.empty()) {
         std::error_code EC;
// Ctx.setDiagnosticHandler(DiagnosticHandler, this);
       EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef());
 
       EmitBackendOutput(Diags, HeaderSearchOpts, CodeGenOpts, TargetOpts,
                         LangOpts, C.getTargetInfo().getDataLayout(),
                         getModule(), Action, std::move(AsmOutStream));
 
       Ctx.setInlineAsmDiagnosticHandler(OldHandler, OldContext);
 
-      Ctx.setDiagnosticHandler(OldDiagnosticHandler, OldDiagnosticContext);
+      //Ctx.setDiagnosticHandler(OldDiagnosticHandler, OldDiagnosticContext);
 
       if (OptRecordFile)
         OptRecordFile->keep();
     }
 
     void HandleTagDeclDefinition(TagDecl *D) override {
       PrettyStackTraceDecl CrashInfo(D, SourceLocation(),
                                      Context->getSourceManager(),
IRGenFinished = true;
     }
 
     static void InlineAsmDiagHandler(const llvm::SMDiagnostic &SM,void *Context,
                                      unsigned LocCookie) {
       SourceLocation Loc = SourceLocation::getFromRawEncoding(LocCookie);
       ((BackendConsumer*)Context)->InlineAsmDiagHandler2(SM, Loc);
     }
 
     static void DiagnosticHandler(const llvm::DiagnosticInfo &DI,
                                   void *Context) {
       ((BackendConsumer *)Context)->DiagnosticHandlerImpl(DI);
     }
 
     /// Get the best possible source location to represent a diagnostic that
     /// may have associated debug info.
     const FullSourceLoc
     getBestLocationFromDebugLoc(const llvm::DiagnosticInfoWithLocationBase &D,
                                 bool &BadDebugInfo, StringRef &Filename,
                                 unsigned &Line, unsigned &Column) const;
 
void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {
   }
 
   // Report the backend message using the usual diagnostic mechanism.
   FullSourceLoc Loc;
   Diags.Report(Loc, DiagID).AddString(MsgStorage);
 }
 #undef ComputeDiagID
 
+class ClangDiagnosticHandler final : public DiagnosticHandler {
+  public:
+  ClangDiagnosticHandler(const CodeGenOptions &CGOpts, void *DiagContext)
+    : DiagnosticHandler(DiagContext), CodeGenOpts(CGOpts) {}
+
+  bool handleDiagnostics(const DiagnosticInfo &DI) override {
+    ((BackendConsumer *)DiagnosticContext)->DiagnosticHandlerImpl(DI);
+    return true;
+  }
+  bool isAnalysisRemarkEnable(const std::string &PassName) {
+  return (CodeGenOpts.OptimizationRemarkAnalysisPattern 
+            && CodeGenOpts.OptimizationRemarkAnalysisPattern->match(PassName));
+  }
+  bool isMissedOptRemarkEnable(const std::string &PassName) {
+    return (CodeGenOpts.OptimizationRemarkMissedPattern 
+            && CodeGenOpts.OptimizationRemarkMissedPattern->match(PassName));
+  }
+  bool isPassedOptRemarkEnable(const std::string &PassName) {
+    return (CodeGenOpts.OptimizationRemarkPattern 
+            && CodeGenOpts.OptimizationRemarkPattern->match(PassName));
+  }
+  private:
+    const CodeGenOptions &CodeGenOpts; 
+};
+
 CodeGenAction::CodeGenAction(unsigned _Act, LLVMContext *_VMContext)
     : Act(_Act), VMContext(_VMContext ? _VMContext : new LLVMContext),
       OwnsVMContext(!_VMContext) {}
 
 CodeGenAction::~CodeGenAction() {
   TheModule.reset();
   if (OwnsVMContext)
     delete VMContext;
CodeGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
   }
 
   std::unique_ptr<BackendConsumer> Result(new BackendConsumer(
       BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),
       CI.getPreprocessorOpts(), CI.getCodeGenOpts(), CI.getTargetOpts(),
       CI.getLangOpts(), CI.getFrontendOpts().ShowTimers, InFile,
       std::move(LinkModules), std::move(OS), *VMContext, CoverageInfo));
   BEConsumer = Result.get();
-
+  VMContext->setDiagnosticHandler(
+    llvm::make_unique<ClangDiagnosticHandler>(CI.getCodeGenOpts(), Result.get()));
   // Enable generating macro debug info only when debug info is not disabled and
   // also macro debug info is enabled.
   if (CI.getCodeGenOpts().getDebugInfo() != codegenoptions::NoDebugInfo &&
       CI.getCodeGenOpts().MacroDebugInfo) {
     std::unique_ptr<PPCallbacks> Callbacks =
         llvm::make_unique<MacroPPCallbacks>(BEConsumer->getCodeGenerator(),
                                             CI.getPreprocessor());
     CI.getPreprocessor().addPPCallbacks(std::move(Callbacks));
   }