From: River Riddle Date: Fri, 3 May 2019 18:40:22 +0000 (-0700) Subject: Add the ability to attach notes to Diagnostic/InFlightDiagnostic. X-Git-Tag: llvmorg-11-init~1466^2~1838 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=baa656352a02accb842c5c416decf2e444212332;p=platform%2Fupstream%2Fllvm.git Add the ability to attach notes to Diagnostic/InFlightDiagnostic. Notes are a way to add additional context to a diagnostic, but don't really make sense as standalone diagnostics. Moving forward, notes will no longer be able to be constructed directly and must be attached to a parent Diagnostic. Notes can be attached via `attachNote`: auto diag = ...; diag.attachNote() << "This is a note"; -- PiperOrigin-RevId: 246545971 --- diff --git a/mlir/include/mlir/IR/Block.h b/mlir/include/mlir/IR/Block.h index 4f33f9e..646e546 100644 --- a/mlir/include/mlir/IR/Block.h +++ b/mlir/include/mlir/IR/Block.h @@ -396,11 +396,10 @@ public: } /// Check that this does not use any value defined outside it. - /// Emit errors if `noteEmitter` is provided; this callback is used to point + /// Emit errors if `noteLoc` is provided; this location is used to point /// to the operation containing the region, the actual error is reported at /// the operation with an offending use. - bool - isIsolatedAbove(llvm::function_ref noteEmitter = {}); + bool isIsolatedAbove(llvm::Optional noteLoc = llvm::None); private: RegionType blocks; diff --git a/mlir/include/mlir/IR/Diagnostics.h b/mlir/include/mlir/IR/Diagnostics.h index ebb70ba..608f2fe 100644 --- a/mlir/include/mlir/IR/Diagnostics.h +++ b/mlir/include/mlir/IR/Diagnostics.h @@ -129,6 +129,22 @@ inline raw_ostream &operator<<(raw_ostream &os, const DiagnosticArgument &arg) { /// to the DiagnosticEngine. It should generally not be constructed directly, /// and instead used transitively via InFlightDiagnostic. class Diagnostic { + using NoteVector = std::vector>; + + /// This class implements a wrapper iterator around NoteVector::iterator to + /// implicitly dereference the unique_ptr. + template + class NoteIteratorImpl + : public llvm::mapped_iterator { + static ResultTy &unwrap(NotePtrTy note) { return *note; } + + public: + NoteIteratorImpl(IteratorTy it) + : llvm::mapped_iterator(it, + &unwrap) {} + }; + public: Diagnostic(Location loc, DiagnosticSeverity severity) : loc(loc), severity(severity) {} @@ -170,6 +186,22 @@ public: /// Converts the diagnostic to a string. std::string str() const; + /// Attaches a note to this diagnostic. A new location may be optionally + /// provided, if not, then the location defaults to the one specified for this + /// diagnostic. Notes may not be attached to other notes. + Diagnostic &attachNote(llvm::Optional noteLoc = llvm::None); + + using note_iterator = NoteIteratorImpl; + using const_note_iterator = NoteIteratorImpl; + + /// Returns the notes held by this diagnostic. + llvm::iterator_range getNotes() { + return {notes.begin(), notes.end()}; + } + llvm::iterator_range getNotes() const { + return {notes.begin(), notes.end()}; + } + private: Diagnostic(const Diagnostic &rhs) = delete; Diagnostic &operator=(const Diagnostic &rhs) = delete; @@ -187,6 +219,9 @@ private: /// those arguments. This is used to guarantee the liveness of non-constant /// strings used in diagnostics. std::vector, unsigned>> stringArguments; + + /// A list of attached notes. + NoteVector notes; }; inline raw_ostream &operator<<(raw_ostream &os, const Diagnostic &diag) { @@ -224,6 +259,12 @@ public: return *this; } + /// Attaches a note to this diagnostic. + Diagnostic &attachNote(llvm::Optional noteLoc = llvm::None) { + assert(isInFlight() && "diagnostic not inflight"); + return impl->attachNote(noteLoc); + } + /// Reports the diagnostic to the engine. void report(); @@ -271,7 +312,7 @@ public: // Diagnostic handler registration and use. MLIR supports the ability for the // IR to carry arbitrary metadata about operation location information. If a // problem is detected by the compiler, it can invoke the emitError / - // emitWarning / emitNote method on an Operation and have it get reported + // emitWarning / emitRemark method on an Operation and have it get reported // through this interface. // // Tools using MLIR are encouraged to register error handlers and define a @@ -292,6 +333,8 @@ public: /// Create a new inflight diagnostic with the given location and severity. InFlightDiagnostic emit(Location loc, DiagnosticSeverity severity) { + assert(severity != DiagnosticSeverity::Note && + "notes should not be emitted directly"); return InFlightDiagnostic(this, Diagnostic(loc, severity)); } diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h index 636122a..08fd4ef 100644 --- a/mlir/include/mlir/IR/OpDefinition.h +++ b/mlir/include/mlir/IR/OpDefinition.h @@ -115,10 +115,6 @@ public: /// handlers that may be listening. InFlightDiagnostic emitWarning(const Twine &message); - /// Emit a note about this operation, reporting up to any diagnostic - /// handlers that may be listening. - InFlightDiagnostic emitNote(const Twine &message); - /// Emit a remark about this operation, reporting up to any diagnostic /// handlers that may be listening. InFlightDiagnostic emitRemark(const Twine &message); @@ -708,9 +704,8 @@ public: NthRegionIsIsolatedAbove::Impl> { public: static LogicalResult verifyTrait(Operation *op) { - auto noteEmitter = [op](const Twine &message) { op->emitNote(message); }; - return op->getRegion(RegionIdx).isIsolatedAbove(noteEmitter) ? success() - : failure(); + return op->getRegion(RegionIdx).isIsolatedAbove(op->getLoc()) ? success() + : failure(); } }; }; diff --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h index 23e147a..908f001 100644 --- a/mlir/include/mlir/IR/Operation.h +++ b/mlir/include/mlir/IR/Operation.h @@ -441,10 +441,6 @@ public: /// handlers that may be listening. InFlightDiagnostic emitWarning(const Twine &message); - /// Emit a note about this operation, reporting up to any diagnostic - /// handlers that may be listening. - InFlightDiagnostic emitNote(const Twine &message); - /// Emit a remark about this operation, reporting up to any diagnostic /// handlers that may be listening. InFlightDiagnostic emitRemark(const Twine &message); diff --git a/mlir/lib/AffineOps/AffineOps.cpp b/mlir/lib/AffineOps/AffineOps.cpp index 7428345..1e26d0e 100644 --- a/mlir/lib/AffineOps/AffineOps.cpp +++ b/mlir/lib/AffineOps/AffineOps.cpp @@ -692,9 +692,10 @@ static LogicalResult checkHasAffineTerminator(OpState &op, Block &block) { return success(); op.emitOpError("expects regions to end with '" + - AffineTerminatorOp::getOperationName() + "'"); - op.emitNote("in custom textual format, the absence of terminator implies '" + - AffineTerminatorOp::getOperationName() + "'"); + AffineTerminatorOp::getOperationName() + "'") + .attachNote() + << "in custom textual format, the absence of terminator implies '" + << AffineTerminatorOp::getOperationName() << "'"; return failure(); } diff --git a/mlir/lib/Analysis/Verifier.cpp b/mlir/lib/Analysis/Verifier.cpp index ee4fd73..6d0b91d 100644 --- a/mlir/lib/Analysis/Verifier.cpp +++ b/mlir/lib/Analysis/Verifier.cpp @@ -349,10 +349,10 @@ LogicalResult FuncVerifier::verifyOpDominance(Operation &op) { if (domInfo->properlyDominates(operand, &op)) continue; - op.emitError("operand #" + Twine(operandNo) + - " does not dominate this use"); + auto diag = op.emitError("operand #" + Twine(operandNo) + + " does not dominate this use"); if (auto *useOp = operand->getDefiningOp()) - useOp->emitNote("operand defined here"); + diag.attachNote(useOp->getLoc()) << "operand defined here"; return failure(); } diff --git a/mlir/lib/IR/Block.cpp b/mlir/lib/IR/Block.cpp index a06a905..a440ca9 100644 --- a/mlir/lib/IR/Block.cpp +++ b/mlir/lib/IR/Block.cpp @@ -353,15 +353,14 @@ void Region::cloneInto(Region *dest, BlockAndValueMapping &mapper, it->walk(remapOperands); } -// Check that the given `region` does not use any value defined outside its -// ancestor region `limit`. That is, given `A{B{C{}}}` with limit `B`, `C` is -// allowed to use values defined in `B` but not those defined in `A`. -// Emit errors if `emitOpNote` is provided; this callback is used to point to -// the operation containing the region, the actual error is reported at the -// operation with an offending use. -static bool -isRegionIsolatedAbove(Region ®ion, Region &limit, - llvm::function_ref emitOpNote = {}) { +/// Check that the given `region` does not use any value defined outside its +/// ancestor region `limit`. That is, given `A{B{C{}}}` with limit `B`, `C` is +/// allowed to use values defined in `B` but not those defined in `A`. +/// Emit errors if `noteLoc` is provided; this location is used to point to +/// the operation containing the region, the actual error is reported at the +/// operation with an offending use. +static bool isRegionIsolatedAbove(Region ®ion, Region &limit, + llvm::Optional noteLoc) { assert(limit.isAncestor(®ion) && "expected isolation limit to be an ancestor of the given region"); @@ -379,9 +378,10 @@ isRegionIsolatedAbove(Region ®ion, Region &limit, // Check that any value that is used by an operation is defined in the // same region as either an operation result or a block argument. if (operand->getContainingRegion()->isProperAncestor(&limit)) { - if (emitOpNote) { - op.emitOpError("using value defined outside the region"); - emitOpNote("required by region isolation constraints"); + if (noteLoc) { + op.emitOpError("using value defined outside the region") + .attachNote(noteLoc) + << "required by region isolation constraints"; } return false; } @@ -397,9 +397,8 @@ isRegionIsolatedAbove(Region ®ion, Region &limit, return true; } -bool Region::isIsolatedAbove( - llvm::function_ref noteEmitter) { - return isRegionIsolatedAbove(*this, *this, noteEmitter); +bool Region::isIsolatedAbove(llvm::Optional noteLoc) { + return isRegionIsolatedAbove(*this, *this, noteLoc); } Region *llvm::ilist_traits<::mlir::Block>::getContainingRegion() { diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp index b266183..ba82040 100644 --- a/mlir/lib/IR/Diagnostics.cpp +++ b/mlir/lib/IR/Diagnostics.cpp @@ -76,6 +76,24 @@ std::string Diagnostic::str() const { return os.str(); } +/// Attaches a note to this diagnostic. A new location may be optionally +/// provided, if not, then the location defaults to the one specified for this +/// diagnostic. Notes may not be attached to other notes. +Diagnostic &Diagnostic::attachNote(llvm::Optional noteLoc) { + // We don't allow attaching notes to notes. + assert(severity != DiagnosticSeverity::Note && + "cannot attach a note to a note"); + + // If a location wasn't provided then reuse our location. + if (!noteLoc) + noteLoc = loc; + + /// Append and return a new note. + notes.push_back( + llvm::make_unique(*noteLoc, DiagnosticSeverity::Note)); + return *notes.back(); +} + //===----------------------------------------------------------------------===// // InFlightDiagnostic //===----------------------------------------------------------------------===// @@ -119,9 +137,6 @@ struct DiagnosticEngineImpl { /// the default behavior if not. void DiagnosticEngineImpl::emit(Location loc, StringRef msg, DiagnosticSeverity severity) { - // Lock access to the handler. - 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 @@ -181,5 +196,12 @@ 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) { + 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()); } diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp index da02c58..2ed3ea3 100644 --- a/mlir/lib/IR/Operation.cpp +++ b/mlir/lib/IR/Operation.cpp @@ -311,13 +311,6 @@ InFlightDiagnostic Operation::emitRemark(const Twine &message) { return getContext()->emitRemark(getLoc(), message); } -/// Emit a note about this operation, reporting up to any diagnostic -/// handlers that may be listening. -InFlightDiagnostic Operation::emitNote(const Twine &message) { - return getContext()->getDiagEngine().emit(getLoc(), DiagnosticSeverity::Note) - << message; -} - /// Emit a warning about this operation, reporting up to any diagnostic /// handlers that may be listening. InFlightDiagnostic Operation::emitWarning(const Twine &message) { @@ -668,12 +661,6 @@ InFlightDiagnostic OpState::emitWarning(const Twine &message) { return getOperation()->emitWarning(message); } -/// Emit a note about this operation, reporting up to any diagnostic -/// handlers that may be listening. -InFlightDiagnostic OpState::emitNote(const Twine &message) { - return getOperation()->emitNote(message); -} - /// Emit a remark about this operation, reporting up to any diagnostic /// handlers that may be listening. InFlightDiagnostic OpState::emitRemark(const Twine &message) {