[Mlir-commits] [mlir] 79afdfa - [mlir] Change the default of `mlir-print-op-on-diagnostic` to true
River Riddle
llvmlistbot at llvm.org
Fri Apr 3 19:07:10 PDT 2020
Author: River Riddle
Date: 2020-04-03T19:02:51-07:00
New Revision: 79afdfab9a57f9ddbc59e9ea0a3ea86635791920
URL: https://github.com/llvm/llvm-project/commit/79afdfab9a57f9ddbc59e9ea0a3ea86635791920
DIFF: https://github.com/llvm/llvm-project/commit/79afdfab9a57f9ddbc59e9ea0a3ea86635791920.diff
LOG: [mlir] Change the default of `mlir-print-op-on-diagnostic` to true
Summary: It is a very common user trap to think that the location printed along with the diagnostic is the same as the current operation that caused the error. This revision changes the behavior to always print the current operation, except for when diagnostics are being verified. This is achieved by moving the command line flags in IR/ to be options on the MLIRContext.
Differential Revision: https://reviews.llvm.org/D77095
Added:
Modified:
mlir/include/mlir/IR/MLIRContext.h
mlir/lib/IR/Diagnostics.cpp
mlir/lib/IR/MLIRContext.cpp
mlir/lib/IR/Operation.cpp
mlir/lib/Support/MlirOptMain.cpp
mlir/tools/mlir-translate/mlir-translate.cpp
Removed:
################################################################################
diff --git a/mlir/include/mlir/IR/MLIRContext.h b/mlir/include/mlir/IR/MLIRContext.h
index 6a65c4e2085c..7df9b765abcb 100644
--- a/mlir/include/mlir/IR/MLIRContext.h
+++ b/mlir/include/mlir/IR/MLIRContext.h
@@ -55,6 +55,22 @@ class MLIRContext {
/// Enables creating operations in unregistered dialects.
void allowUnregisteredDialects(bool allow = true);
+ /// Return true if we should attach the operation to diagnostics emitted via
+ /// Operation::emit.
+ bool shouldPrintOpOnDiagnostic();
+
+ /// Set the flag specifying if we should attach the operation to diagnostics
+ /// emitted via Operation::emit.
+ void printOpOnDiagnostic(bool enable);
+
+ /// Return true if we should attach the current stacktrace to diagnostics when
+ /// emitted.
+ bool shouldPrintStackTraceOnDiagnostic();
+
+ /// Set the flag specifying if we should attach the current stacktrace when
+ /// emitting diagnostics.
+ void printStackTraceOnDiagnostic(bool enable);
+
/// Return information about all registered operations. This isn't very
/// efficient: typically you should ask the operations about their properties
/// directly.
diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp
index 1acb7e7f5931..f4e301761da8 100644
--- a/mlir/lib/IR/Diagnostics.cpp
+++ b/mlir/lib/IR/Diagnostics.cpp
@@ -16,7 +16,6 @@
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
-#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Mutex.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/Regex.h"
@@ -27,11 +26,6 @@
using namespace mlir;
using namespace mlir::detail;
-static llvm::cl::opt<bool> printStackTraceOnDiagnostic(
- "mlir-print-stacktrace-on-diagnostic",
- llvm::cl::desc("When a diagnostic is emitted, also print the stack trace "
- "as an attached note"));
-
//===----------------------------------------------------------------------===//
// DiagnosticArgument
//===----------------------------------------------------------------------===//
@@ -278,13 +272,14 @@ void DiagnosticEngine::emit(Diagnostic diag) {
/// diagnostic.
static InFlightDiagnostic
emitDiag(Location location, DiagnosticSeverity severity, const Twine &message) {
- auto &diagEngine = location->getContext()->getDiagEngine();
+ MLIRContext *ctx = location->getContext();
+ auto &diagEngine = ctx->getDiagEngine();
auto diag = diagEngine.emit(location, severity);
if (!message.isTriviallyEmpty())
diag << message;
// Add the stack trace as a note if necessary.
- if (printStackTraceOnDiagnostic) {
+ if (ctx->shouldPrintStackTraceOnDiagnostic()) {
std::string bt;
{
llvm::raw_string_ostream stream(bt);
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index 3875b57775bc..2407c8f30c4f 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -31,6 +31,7 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/RWMutex.h"
#include "llvm/Support/raw_ostream.h"
#include <memory>
@@ -41,6 +42,17 @@ using namespace mlir::detail;
using llvm::hash_combine;
using llvm::hash_combine_range;
+static llvm::cl::opt<bool> clPrintOpOnDiagnostic(
+ "mlir-print-op-on-diagnostic",
+ llvm::cl::desc("When a diagnostic is emitted on an operation, also print "
+ "the operation as an attached note"),
+ llvm::cl::init(true));
+
+static llvm::cl::opt<bool> clPrintStackTraceOnDiagnostic(
+ "mlir-print-stacktrace-on-diagnostic",
+ llvm::cl::desc("When a diagnostic is emitted, also print the stack trace "
+ "as an attached note"));
+
/// A utility function to safely get or create a uniqued instance within the
/// given set container.
template <typename ValueT, typename DenseInfoT, typename KeyT,
@@ -170,6 +182,13 @@ class MLIRContextImpl {
/// detect such use cases
bool allowUnregisteredDialects = false;
+ /// If the operation should be attached to diagnostics printed via the
+ /// Operation::emit methods.
+ bool printOpOnDiagnostic;
+
+ /// If the current stack trace should be attached when emitting diagnostics.
+ bool printStackTraceOnDiagnostic;
+
//===--------------------------------------------------------------------===//
// Other
//===--------------------------------------------------------------------===//
@@ -234,7 +253,10 @@ class MLIRContextImpl {
UnknownLoc unknownLocAttr;
public:
- MLIRContextImpl() : identifiers(identifierAllocator) {}
+ MLIRContextImpl()
+ : printOpOnDiagnostic(clPrintOpOnDiagnostic),
+ printStackTraceOnDiagnostic(clPrintStackTraceOnDiagnostic),
+ identifiers(identifierAllocator) {}
};
} // end namespace mlir
@@ -366,6 +388,34 @@ void MLIRContext::allowUnregisteredDialects(bool allowing) {
impl->allowUnregisteredDialects = allowing;
}
+/// Return true if we should attach the operation to diagnostics emitted via
+/// Operation::emit.
+bool MLIRContext::shouldPrintOpOnDiagnostic() {
+ return impl->printOpOnDiagnostic;
+}
+
+/// Set the flag specifying if we should attach the operation to diagnostics
+/// emitted via Operation::emit.
+void MLIRContext::printOpOnDiagnostic(bool enable) {
+ // Let the command line option take priority.
+ if (!clPrintOpOnDiagnostic.getNumOccurrences())
+ impl->printOpOnDiagnostic = enable;
+}
+
+/// Return true if we should attach the current stacktrace to diagnostics when
+/// emitted.
+bool MLIRContext::shouldPrintStackTraceOnDiagnostic() {
+ return impl->printStackTraceOnDiagnostic;
+}
+
+/// Set the flag specifying if we should attach the current stacktrace when
+/// emitting diagnostics.
+void MLIRContext::printStackTraceOnDiagnostic(bool enable) {
+ // Let the command line option take priority.
+ if (!clPrintStackTraceOnDiagnostic.getNumOccurrences())
+ impl->printStackTraceOnDiagnostic = enable;
+}
+
/// Return information about all registered operations. This isn't very
/// efficient, typically you should ask the operations about their properties
/// directly.
diff --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index 58ebf29ceebd..b94ee96f8ffa 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -13,16 +13,10 @@
#include "mlir/IR/PatternMatch.h"
#include "mlir/IR/StandardTypes.h"
#include "mlir/IR/TypeUtilities.h"
-#include "llvm/Support/CommandLine.h"
#include <numeric>
using namespace mlir;
-static llvm::cl::opt<bool> printOpOnDiagnostic(
- "mlir-print-op-on-diagnostic",
- llvm::cl::desc("When a diagnostic is emitted on an operation, also print "
- "the operation as an attached note"));
-
OpAsmParser::~OpAsmParser() {}
//===----------------------------------------------------------------------===//
@@ -252,7 +246,7 @@ void Operation::setOperands(ValueRange operands) {
/// any diagnostic handlers that may be listening.
InFlightDiagnostic Operation::emitError(const Twine &message) {
InFlightDiagnostic diag = mlir::emitError(getLoc(), message);
- if (printOpOnDiagnostic) {
+ if (getContext()->shouldPrintOpOnDiagnostic()) {
// Print out the operation explicitly here so that we can print the generic
// form.
// TODO(riverriddle) It would be nice if we could instead provide the
@@ -272,7 +266,7 @@ InFlightDiagnostic Operation::emitError(const Twine &message) {
/// handlers that may be listening.
InFlightDiagnostic Operation::emitWarning(const Twine &message) {
InFlightDiagnostic diag = mlir::emitWarning(getLoc(), message);
- if (printOpOnDiagnostic)
+ if (getContext()->shouldPrintOpOnDiagnostic())
diag.attachNote(getLoc()) << "see current operation: " << *this;
return diag;
}
@@ -281,7 +275,7 @@ InFlightDiagnostic Operation::emitWarning(const Twine &message) {
/// handlers that may be listening.
InFlightDiagnostic Operation::emitRemark(const Twine &message) {
InFlightDiagnostic diag = mlir::emitRemark(getLoc(), message);
- if (printOpOnDiagnostic)
+ if (getContext()->shouldPrintOpOnDiagnostic())
diag.attachNote(getLoc()) << "see current operation: " << *this;
return diag;
}
diff --git a/mlir/lib/Support/MlirOptMain.cpp b/mlir/lib/Support/MlirOptMain.cpp
index 53e336a24b47..5c21e19c4bd2 100644
--- a/mlir/lib/Support/MlirOptMain.cpp
+++ b/mlir/lib/Support/MlirOptMain.cpp
@@ -76,6 +76,7 @@ static LogicalResult processBuffer(raw_ostream &os,
// Parse the input file.
MLIRContext context;
context.allowUnregisteredDialects(allowUnregisteredDialects);
+ context.printOpOnDiagnostic(!verifyDiagnostics);
// If we are in verify diagnostics mode then we have a lot of work to do,
// otherwise just perform the actions without worrying about it.
diff --git a/mlir/tools/mlir-translate/mlir-translate.cpp b/mlir/tools/mlir-translate/mlir-translate.cpp
index 1efcaec9cd63..d7c1eb626952 100644
--- a/mlir/tools/mlir-translate/mlir-translate.cpp
+++ b/mlir/tools/mlir-translate/mlir-translate.cpp
@@ -74,6 +74,7 @@ int main(int argc, char **argv) {
raw_ostream &os) {
MLIRContext context;
context.allowUnregisteredDialects();
+ context.printOpOnDiagnostic(!verifyDiagnostics);
llvm::SourceMgr sourceMgr;
sourceMgr.AddNewSourceBuffer(std::move(ownedBuffer), llvm::SMLoc());
More information about the Mlir-commits
mailing list