[mlir] Change the default of `mlir-print-op-on-diagnostic` to true

[rriddle]

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.

[mehdi_amini]

mlir/lib/IR/MLIRContext.cpp

55

Can we avoid cl::opt here and push it to clients? (trying to avoid more global constructor / global state)

[rriddle]

mlir/lib/IR/MLIRContext.cpp

55

This is the same situation that we are already in. Moving to clients will require more churn on a currently simple revision. I'd rather defer that for a followup.

[rriddle]

Rebase

mlir/include/mlir/IR/MLIRContext.h

public:
   }
 
   /// Return true if we allow to create operation for unregistered dialects.
   bool allowsUnregisteredDialects();
 
   /// 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.
   std::vector<AbstractOperation *> getRegisteredOperations();
 
   // This is effectively private given that only MLIRContext.cpp can see the
   // MLIRContextImpl type.
   MLIRContextImpl &getImpl() { return *impl; }

mlir/lib/IR/Diagnostics.cpp

 #include "mlir/IR/Identifier.h"
 #include "mlir/IR/Location.h"
 #include "mlir/IR/MLIRContext.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/IR/Types.h"
 #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"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"
 
 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
 //===----------------------------------------------------------------------===//
 
 // Construct from an Attribute.
 DiagnosticArgument::DiagnosticArgument(Attribute attr)
     : kind(DiagnosticArgumentKind::Attribute),
       opaqueVal(reinterpret_cast<intptr_t>(attr.getAsOpaquePointer())) {}
void DiagnosticEngine::emit(Diagnostic diag) {
   impl->emit(std::move(diag));
 }
 
 /// Helper function used to emit a diagnostic with an optionally empty twine
 /// message. If the message is empty, then it is not inserted into the
 /// 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);
       llvm::sys::PrintStackTrace(stream);
     }
     if (!bt.empty())
       diag.attachNote() << "diagnostic emitted with trace:\n" << bt;
   }

mlir/lib/IR/MLIRContext.cpp

 #include "mlir/IR/Types.h"
 #include "mlir/Support/STLExtras.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SetVector.h"
 #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>
 
 using namespace mlir;
 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,
           typename ConstructorFn>
 static ValueT safeGetOrCreate(DenseSet<ValueT, DenseInfoT> &container,
                               KeyT &&key, llvm::sys::SmartRWMutex<true> &mutex,
                               ConstructorFn &&constructorFn) {
   { // Check for an existing instance in read-only mode.
public:
   // Options
   //===--------------------------------------------------------------------===//
 
   /// In most cases, creating operation in unregistered dialect is not desired
   /// and indicate a misconfiguration of the compiler. This option enables to
   /// 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
   //===--------------------------------------------------------------------===//
 
   /// A general purpose mutex to lock access to parts of the context that do not
   /// have a more specific mutex, e.g. registry operations.
   llvm::sys::SmartRWMutex<true> contextMutex;
 
public:
   StorageUniquer attributeUniquer;
 
   /// Cached Attribute Instances.
   BoolAttr falseAttr, trueAttr;
   UnitAttr unitAttr;
   UnknownLoc unknownLocAttr;
 
 public:
-  MLIRContextImpl() : identifiers(identifierAllocator) {}
+  MLIRContextImpl()
+      : printOpOnDiagnostic(clPrintOpOnDiagnostic),
+        printStackTraceOnDiagnostic(clPrintStackTraceOnDiagnostic),
+        identifiers(identifierAllocator) {}
 };
 } // end namespace mlir
 
 MLIRContext::MLIRContext() : impl(new MLIRContextImpl()) {
   new BuiltinDialect(this);
   registerAllDialects(this);
 
   // Initialize several common attributes and types to avoid the need to lock

 bool MLIRContext::allowsUnregisteredDialects() {
   return impl->allowUnregisteredDialects;
 }
 
 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.
 std::vector<AbstractOperation *> MLIRContext::getRegisteredOperations() {
   std::vector<std::pair<StringRef, AbstractOperation *>> opsToSort;
 
   { // Lock access to the context registry.
     llvm::sys::SmartScopedReader<true> registryLock(getImpl().contextMutex);

mlir/lib/IR/Operation.cpp

 //===- Operation.cpp - Operation support code -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/IR/Operation.h"
 #include "mlir/IR/BlockAndValueMapping.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/OpImplementation.h"
 #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() {}
 
 //===----------------------------------------------------------------------===//
 // OperationName
 //===----------------------------------------------------------------------===//
 
 /// Form the OperationName for an op with the specified string.  This either is
 /// a reference to an AbstractOperation if one is known, or a uniqued Identifier

 //===----------------------------------------------------------------------===//
 // Diagnostics
 //===----------------------------------------------------------------------===//
 
 /// Emit an error about fatal conditions with this operation, reporting up to
 /// 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
     // specific printing flags when adding the operation as an argument to the
     // diagnostic.
     std::string printedOp;
     {
       llvm::raw_string_ostream os(printedOp);
       print(os, OpPrintingFlags().printGenericOpForm().useLocalScope());
     }
     diag.attachNote(getLoc()) << "see current operation: " << printedOp;
   }
   return diag;
 }
 
 /// Emit a warning about this operation, reporting up to any diagnostic
 /// 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;
 }
 
 /// Emit a remark about this operation, reporting up to any diagnostic
 /// 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;
 }
 
 //===----------------------------------------------------------------------===//
 // Operation Ordering
 //===----------------------------------------------------------------------===//
 

mlir/lib/Support/MlirOptMain.cpp

static LogicalResult processBuffer(raw_ostream &os,
                                    const PassPipelineCLParser &passPipeline) {
   // Tell sourceMgr about this buffer, which is what the parser will pick up.
   SourceMgr sourceMgr;
   sourceMgr.AddNewSourceBuffer(std::move(ownedBuffer), SMLoc());
 
   // 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.
   if (!verifyDiagnostics) {
     SourceMgrDiagnosticHandler sourceMgrHandler(sourceMgr, &context);
     return performActions(os, verifyDiagnostics, verifyPasses, sourceMgr,
                           &context, passPipeline);
   }

mlir/tools/mlir-translate/mlir-translate.cpp

if (!output) {
     return 1;
   }
 
   // Processes the memory buffer with a new MLIRContext.
   auto processBuffer = [&](std::unique_ptr<llvm::MemoryBuffer> ownedBuffer,
                            raw_ostream &os) {
     MLIRContext context;
     context.allowUnregisteredDialects();
+    context.printOpOnDiagnostic(!verifyDiagnostics);
     llvm::SourceMgr sourceMgr;
     sourceMgr.AddNewSourceBuffer(std::move(ownedBuffer), llvm::SMLoc());
 
     if (!verifyDiagnostics) {
       SourceMgrDiagnosticHandler sourceMgrHandler(sourceMgr, &context);
       return (*translationRequested)(sourceMgr, os, &context);
     }