From 85bf79851eb1030afb8179b391bcdeec82710968 Mon Sep 17 00:00:00 2001 From: River Riddle Date: Fri, 10 May 2019 17:47:32 -0700 Subject: [PATCH] Change the diagnostic handler to accept Diagnostic instead of location/message/kind. This opens the door for many more powerful use cases: fixits, colors, etc. -- PiperOrigin-RevId: 247705673 --- mlir/include/mlir/IR/Diagnostics.h | 16 +++--- mlir/lib/IR/Diagnostics.cpp | 89 ++++++++++++++---------------- mlir/lib/Pass/Pass.cpp | 81 +++++++++++++-------------- 3 files changed, 87 insertions(+), 99 deletions(-) diff --git a/mlir/include/mlir/IR/Diagnostics.h b/mlir/include/mlir/IR/Diagnostics.h index 9ab29c7aec14..3360e4b82f1e 100644 --- a/mlir/include/mlir/IR/Diagnostics.h +++ b/mlir/include/mlir/IR/Diagnostics.h @@ -404,13 +404,10 @@ public: // schema for their location information. If they don't, then warnings and // notes will be dropped and errors will be emitted to errs. - using HandlerTy = - std::function; + using HandlerTy = std::function; - /// Set the diagnostic handler for this engine. The handler is passed - /// location information if present (nullptr if not) along with a message and - /// a severity that indicates whether this is an error, warning, etc. Note - /// that this replaces any existing handler. + /// Set the diagnostic handler for this engine. Note that this replaces any + /// existing handler. void setHandler(const HandlerTy &handler); /// Return the current diagnostic handler, or null if none is present. @@ -423,9 +420,9 @@ public: return InFlightDiagnostic(this, Diagnostic(loc, severity)); } - /// Emit a diagnostic using the registered issue handle if present, or with + /// Emit a diagnostic using the registered issue handler if present, or with /// the default behavior if not. - void emit(const Diagnostic &diag); + void emit(Diagnostic diag); private: friend class MLIRContextImpl; @@ -485,6 +482,9 @@ public: LogicalResult verify(); private: + /// Process a single diagnostic. + void process(Diagnostic &diag); + /// Process a FileLineColLoc diagnostic. void process(FileLineColLoc loc, StringRef msg, DiagnosticSeverity kind); diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp index 10eaed856393..b03a30d655fb 100644 --- a/mlir/lib/IR/Diagnostics.cpp +++ b/mlir/lib/IR/Diagnostics.cpp @@ -141,7 +141,7 @@ void InFlightDiagnostic::report() { // If this diagnostic is still inflight and it hasn't been abandoned, then // report it. if (isInFlight()) { - owner->emit(*impl); + owner->emit(std::move(*impl)); owner = nullptr; } impl.reset(); @@ -159,7 +159,7 @@ namespace detail { struct DiagnosticEngineImpl { /// Emit a diagnostic using the registered issue handle if present, or with /// the default behavior if not. - void emit(Location loc, StringRef msg, DiagnosticSeverity severity); + void emit(Diagnostic diag); /// A mutex to ensure that diagnostics emission is thread-safe. llvm::sys::SmartMutex mutex; @@ -173,39 +173,24 @@ struct DiagnosticEngineImpl { /// Emit a diagnostic using the registered issue handle if present, or with /// the default behavior if not. -void DiagnosticEngineImpl::emit(Location loc, StringRef msg, - DiagnosticSeverity severity) { +void DiagnosticEngineImpl::emit(Diagnostic diag) { + llvm::sys::SmartScopedLock lock(mutex); + // If we had a handler registered, emit the diagnostic using it. - if (handler) { - // TODO(b/131756158) FusedLoc should be handled by the diagnostic handler - // instead of here. - // Check to see if we are emitting a diagnostic on a fused location. - if (auto fusedLoc = loc.dyn_cast()) { - auto fusedLocs = fusedLoc->getLocations(); - - // Emit the original diagnostic with the first location in the fused list. - emit(fusedLocs.front(), msg, severity); - - // Emit the rest of the locations as notes. - for (Location subLoc : fusedLocs.drop_front()) - emit(subLoc, "fused from here", DiagnosticSeverity::Note); - return; - } - - return handler(loc, msg, severity); - } + if (handler) + return handler(std::move(diag)); // Otherwise, if this is an error we emit it to stderr. - if (severity != DiagnosticSeverity::Error) + if (diag.getSeverity() != DiagnosticSeverity::Error) return; auto &os = llvm::errs(); - if (!loc.isa()) - os << loc << ": "; + if (!diag.getLocation().isa()) + os << diag.getLocation() << ": "; os << "error: "; // The default behavior for errors is to emit them to stderr. - os << msg << '\n'; + os << diag << '\n'; os.flush(); } @@ -221,7 +206,6 @@ DiagnosticEngine::~DiagnosticEngine() {} /// a severity that indicates whether this is an error, warning, etc. Note /// that this replaces any existing handler. void DiagnosticEngine::setHandler(const HandlerTy &handler) { - llvm::sys::SmartScopedLock lock(impl->mutex); impl->handler = handler; } @@ -233,15 +217,10 @@ auto DiagnosticEngine::getHandler() -> HandlerTy { /// Emit a diagnostic using the registered issue handler if present, or with /// the default behavior if not. -void DiagnosticEngine::emit(const Diagnostic &diag) { +void DiagnosticEngine::emit(Diagnostic diag) { assert(diag.getSeverity() != DiagnosticSeverity::Note && "notes should not be emitted directly"); - llvm::sys::SmartScopedLock lock(impl->mutex); - impl->emit(diag.getLocation(), diag.str(), diag.getSeverity()); - - // Emit any notes that were attached to this diagnostic. - for (auto ¬e : diag.getNotes()) - impl->emit(note.getLocation(), note.str(), note.getSeverity()); + impl->emit(std::move(diag)); } //===----------------------------------------------------------------------===// @@ -287,10 +266,14 @@ SourceMgrDiagnosticHandler::SourceMgrDiagnosticHandler(llvm::SourceMgr &mgr, MLIRContext *ctx) : mgr(mgr) { // Register a simple diagnostic handler. - ctx->getDiagEngine().setHandler( - [this](Location loc, StringRef message, DiagnosticSeverity kind) { - emitDiagnostic(loc, message, kind); - }); + ctx->getDiagEngine().setHandler([this](Diagnostic diag) { + // Emit the diagnostic. + emitDiagnostic(diag.getLocation(), diag.str(), diag.getSeverity()); + + // Emit each of the notes. + for (auto ¬e : diag.getNotes()) + emitDiagnostic(note.getLocation(), note.str(), note.getSeverity()); + }); } void SourceMgrDiagnosticHandler::emitDiagnostic(Location loc, Twine message, @@ -498,16 +481,14 @@ SourceMgrDiagnosticVerifierHandler::SourceMgrDiagnosticVerifierHandler( (void)impl->computeExpectedDiags(mgr.getMemoryBuffer(i + 1)); // Register a handler to verfy the diagnostics. - ctx->getDiagEngine().setHandler( - [&](Location loc, StringRef msg, DiagnosticSeverity kind) { - // Process a FileLineColLoc. - if (auto fileLoc = getFileLineColLoc(loc)) - return process(*fileLoc, msg, kind); + ctx->getDiagEngine().setHandler([&](Diagnostic diag) { + // Process the main diagnostics. + process(diag); - emitDiagnostic(loc, "unexpected " + getDiagKindStr(kind) + ": " + msg, - DiagnosticSeverity::Error); - impl->status = failure(); - }); + // Process each of the notes. + for (auto ¬e : diag.getNotes()) + process(note); + }); } SourceMgrDiagnosticVerifierHandler::~SourceMgrDiagnosticVerifierHandler() { @@ -538,6 +519,20 @@ LogicalResult SourceMgrDiagnosticVerifierHandler::verify() { return impl->status; } +/// Process a single diagnostic. +void SourceMgrDiagnosticVerifierHandler::process(Diagnostic &diag) { + auto kind = diag.getSeverity(); + + // Process a FileLineColLoc. + if (auto fileLoc = getFileLineColLoc(diag.getLocation())) + return process(*fileLoc, diag.str(), kind); + + emitDiagnostic(diag.getLocation(), + "unexpected " + getDiagKindStr(kind) + ": " + diag.str(), + DiagnosticSeverity::Error); + impl->status = failure(); +} + /// Process a FileLineColLoc diagnostic. void SourceMgrDiagnosticVerifierHandler::process(FileLineColLoc loc, StringRef msg, diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp index 2489115877d7..213bf51cc4b7 100644 --- a/mlir/lib/Pass/Pass.cpp +++ b/mlir/lib/Pass/Pass.cpp @@ -183,9 +183,8 @@ namespace { /// ModuleToFunctionPassAdaptorParallel. struct ParallelDiagnosticHandler { struct ThreadDiagnostic { - ThreadDiagnostic(size_t id, Location loc, StringRef msg, - DiagnosticSeverity kind) - : id(id), loc(loc), msg(msg), kind(kind) {} + ThreadDiagnostic(size_t id, Diagnostic diag) + : id(id), diag(std::move(diag)) {} bool operator<(const ThreadDiagnostic &rhs) const { return id < rhs.id; } /// The function id for this diagnostic, this is used for ordering. @@ -193,22 +192,19 @@ struct ParallelDiagnosticHandler { /// function within its parent module. size_t id; - /// Information for the diagnostic. - Location loc; - std::string msg; - DiagnosticSeverity kind; + /// The diagnostic. + Diagnostic diag; }; ParallelDiagnosticHandler(MLIRContext &ctx) : prevHandler(ctx.getDiagEngine().getHandler()), context(ctx) { - ctx.getDiagEngine().setHandler( - [this](Location loc, StringRef message, DiagnosticSeverity kind) { - uint64_t tid = llvm::get_threadid(); - llvm::sys::SmartScopedLock lock(mutex); + ctx.getDiagEngine().setHandler([this](Diagnostic diag) { + uint64_t tid = llvm::get_threadid(); + llvm::sys::SmartScopedLock lock(mutex); - // Append a new diagnostic. - diagnostics.emplace_back(threadToFuncID[tid], loc, message, kind); - }); + // Append a new diagnostic. + diagnostics.emplace_back(threadToFuncID[tid], std::move(diag)); + }); } ~ParallelDiagnosticHandler() { @@ -220,15 +216,13 @@ struct ParallelDiagnosticHandler { return; // Emit the diagnostics back to the context. - emitDiagnostics( - [&](Location loc, StringRef message, DiagnosticSeverity kind) { - return context.getDiagEngine().emit(loc, kind) << message; - }); + emitDiagnostics([&](Diagnostic diag) { + return context.getDiagEngine().emit(std::move(diag)); + }); } /// Utility method to emit any held diagnostics. - void emitDiagnostics( - std::function emitFn) { + void emitDiagnostics(std::function emitFn) { // Stable sort all of the diagnostics that were emitted. This creates a // deterministic ordering for the diagnostics based upon which function they // were emitted for. @@ -236,7 +230,7 @@ struct ParallelDiagnosticHandler { // Emit each diagnostic to the context again. for (ThreadDiagnostic &diag : diagnostics) - emitFn(diag.loc, diag.msg, diag.kind); + emitFn(std::move(diag.diag)); } /// Set the function id for the current thread. @@ -276,30 +270,29 @@ struct PrettyStackTraceParallelDiagnosticEntry return; os << "In-Flight Diagnostics:\n"; - parallelHandler.emitDiagnostics( - [&](Location loc, StringRef message, DiagnosticSeverity severity) { - os.indent(4); + parallelHandler.emitDiagnostics([&](Diagnostic diag) { + os.indent(4); - // Print each diagnostic with the format: - // ": : " - if (!loc.isa()) - os << loc << ": "; - switch (severity) { - case DiagnosticSeverity::Error: - os << "error: "; - break; - case DiagnosticSeverity::Warning: - os << "warning: "; - break; - case DiagnosticSeverity::Note: - os << "note: "; - break; - case DiagnosticSeverity::Remark: - os << "remark: "; - break; - } - os << message << '\n'; - }); + // Print each diagnostic with the format: + // ": : " + if (!diag.getLocation().isa()) + os << diag.getLocation() << ": "; + switch (diag.getSeverity()) { + case DiagnosticSeverity::Error: + os << "error: "; + break; + case DiagnosticSeverity::Warning: + os << "warning: "; + break; + case DiagnosticSeverity::Note: + os << "note: "; + break; + case DiagnosticSeverity::Remark: + os << "remark: "; + break; + } + os << diag << '\n'; + }); } // A reference to the parallel handler to dump on the event of a crash.