Page MenuHomePhabricator

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

Authored by vivekvpandya on Aug 27 2017, 8:59 AM.

Details

Reviewers
anemet
hfinkel
Summary

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

Diff Detail

Event Timeline

vivekvpandya created this revision.Aug 27 2017, 8:59 AM

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.

Clang-formated

anemet edited edge metadata.Sep 8 2017, 10:19 AM

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
326–327

Remove this.

786

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

881–882

Any reason you moved where we set this up?

vivekvpandya marked an inline comment as done.Sep 11 2017, 11:34 AM
vivekvpandya added inline comments.
lib/CodeGen/CodeGenAction.cpp
881–882
  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.

anemet added inline comments.Sep 11 2017, 9:50 PM
lib/CodeGen/CodeGenAction.cpp
881–882

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.

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.

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.

anemet accepted this revision.Sep 13 2017, 4:14 PM

LGTM.

This revision is now accepted and ready to land.Sep 13 2017, 4:14 PM