[MLIR] Fix assert crash when an unregistered dialect op is encountered

[bondhugula]

Summary

Fix assert crash when an unregistered dialect op is encountered during
parsing and `-allow-unregistered-dialect' isn't on. Instead, emit an
error.

While on this, clean up "registered" vs "loaded" on getDialect() and
local clang-tidy warnings.

https://llvm.discourse.group/t/assert-behavior-on-unregistered-dialect-ops/4402

[bondhugula]

Update comment.

[lattner]

Awesome, thanks for fixing this!

[mehdi_amini]

Thanks Uday!

mlir/include/mlir/IR/Operation.h

public:
 
   /// Returns the operation block that contains this operation.
   Block *getBlock() { return block; }
 
   /// Return the context this operation is associated with.
   MLIRContext *getContext() { return location->getContext(); }
 
   /// Return the dialect this operation is associated with, or nullptr if the
-  /// associated dialect is not registered.
+  /// associated dialect is not loaded.
   Dialect *getDialect() { return getName().getDialect(); }
 
   /// The source location the operation was defined or derived from.
   Location getLoc() { return location; }
 
   /// Set the source location the operation was defined or derived from.
   void setLoc(Location loc) { location = loc; }
 

mlir/include/mlir/IR/OperationSupport.h

using RepresentationUnion =
       PointerUnion<Identifier, const AbstractOperation *>;
 
   OperationName(AbstractOperation *op) : representation(op) {}
   OperationName(StringRef name, MLIRContext *context);
 
   /// Return the name of the dialect this operation is registered to.
   StringRef getDialectNamespace() const;
 
-  /// Return the Dialect this operation is registered to if it is loaded in the
-  /// context, or nullptr if the dialect isn't loaded.
+  /// Return the dialect this operation is registered to if the dialect is
+  /// loaded in the context, or nullptr if the dialect isn't loaded.
   Dialect *getDialect() const {
     if (const auto *abstractOp = getAbstractOperation())
       return &abstractOp->dialect;
     return representation.get<Identifier>().getDialect();
   }
 
   /// Return the operation name with dialect name stripped, if it has one.
   StringRef stripDialect() const;

mlir/lib/IR/Operation.cpp

: location(location), numResults(numResults), numSuccs(numSuccessors),
       attrs(attributes) {
   assert(attributes && "unexpected null attribute dictionary");
 #ifndef NDEBUG
   if (!getDialect() && !getContext()->allowsUnregisteredDialects())
     llvm::report_fatal_error(
         name.getStringRef() +
         " created with unregistered dialect. If this is intended, please call "
         "allowUnregisteredDialects() on the MLIRContext, or use "
-        "-allow-unregistered-dialect with the MLIR opt tool used");
+        "-allow-unregistered-dialect with the MLIR tool used.");
 #endif
 }
 
 // Operations are deleted through the destroy() member because they are
 // allocated via malloc.
 Operation::~Operation() {
   assert(block == nullptr && "operation destroyed but still in a block");
 #ifndef NDEBUG

mlir/lib/Parser/Parser.cpp

ParseResult OperationParser::finalize() {
   // out.
   if (!forwardRefPlaceholders.empty()) {
     SmallVector<const char *, 4> errors;
     // Iteration over the map isn't deterministic, so sort by source location.
     for (auto entry : forwardRefPlaceholders)
       errors.push_back(entry.second.getPointer());
     llvm::array_pod_sort(errors.begin(), errors.end());
 
-    for (auto entry : errors) {
+    for (const char *entry : errors) {
       auto loc = SMLoc::getFromPointer(entry);
       emitError(loc, "use of undeclared SSA value name");
     }
     return failure();
   }
 
   // Resolve the locations of any deferred operations.
   auto &attributeAliases = state.symbols.attributeAliasDefinitions;
Operation *OperationParser::parseGenericOperation() {
 
   consumeToken(Token::string);
 
   OperationState result(srcLocation, name);
 
   // Lazy load dialects in the context as needed.
   if (!result.name.getAbstractOperation()) {
     StringRef dialectName = StringRef(name).split('.').first;
-    if (!getContext()->getLoadedDialect(dialectName) &&
-        getContext()->getOrLoadDialect(dialectName)) {
+    if (!getContext()->getLoadedDialect(dialectName)) {
+      if (getContext()->getOrLoadDialect(dialectName)) {
         result.name = OperationName(name, getContext());
+      } else if (!getContext()->allowsUnregisteredDialects()) {
+        // Emit an error if the dialect couldn't be loaded (i.e., it was not
+        // registered) and unregistered dialects aren't allowed).
+        return emitError(
+                   "operation being parsed with an unregistered dialect. If "
+                   "this is intended, please use -allow-unregistered-dialect "
+                   "with the MLIR tool used"),
+               nullptr;
+      }
     }
   }
 
   // If we are populating the parser state, start a new operation definition.
   if (state.asmState)
     state.asmState->startOperationDefinition(result.name);
 
   // Parse the operand list.
ParseResult OperationParser::parseRegionBody(
     ArrayRef<std::pair<OperationParser::SSAUseInfo, Type>> entryArguments,
     bool isIsolatedNameScope) {
   auto currentPt = opBuilder.saveInsertionPoint();
 
   // Push a new named value scope.
   pushSSANameScope(isIsolatedNameScope);
 
   // Parse the first block directly to allow for it to be unnamed.
-  auto owning_block = std::make_unique<Block>();
-  Block *block = owning_block.get();
+  auto owningBlock = std::make_unique<Block>();
+  Block *block = owningBlock.get();
 
   // If this block is not defined in the source file, add a definition for it
   // now in the assembly state. Blocks with a name will be defined when the name
   // is parsed.
   if (state.asmState && getToken().isNot(Token::caret_identifier))
     state.asmState->addDefinition(block, startLoc);
 
   // Add arguments to the entry block.
ParseResult OperationParser::parseRegionBody(
 
   // Verify that no other arguments were parsed.
   if (!entryArguments.empty() &&
       block->getNumArguments() > entryArguments.size()) {
     return emitError("entry block arguments were already defined");
   }
 
   // Parse the rest of the region.
-  region.push_back(owning_block.release());
+  region.push_back(owningBlock.release());
   while (getToken().isNot(Token::r_brace)) {
     Block *newBlock = nullptr;
     if (parseBlock(newBlock))
       return failure();
     region.push_back(newBlock);
   }
 
   // Pop the SSA value scope for this region.
LogicalResult mlir::parseSourceFile(llvm::StringRef filename,
                                     MLIRContext *context,
                                     LocationAttr *sourceFileLoc,
                                     AsmParserState *asmState) {
   if (sourceMgr.getNumBuffers() != 0) {
     // TODO: Extend to support multiple buffers.
     return emitError(mlir::UnknownLoc::get(context),
                      "only main buffer parsed at the moment");
   }
-  auto file_or_err = llvm::MemoryBuffer::getFileOrSTDIN(filename);
-  if (std::error_code error = file_or_err.getError())
+  auto fileOrErr = llvm::MemoryBuffer::getFileOrSTDIN(filename);
+  if (std::error_code error = fileOrErr.getError())
     return emitError(mlir::UnknownLoc::get(context),
                      "could not open input file " + filename);
 
   // Load the MLIR source file.
-  sourceMgr.AddNewSourceBuffer(std::move(*file_or_err), llvm::SMLoc());
+  sourceMgr.AddNewSourceBuffer(std::move(*fileOrErr), llvm::SMLoc());
   return parseSourceFile(sourceMgr, block, context, sourceFileLoc, asmState);
 }
 
 LogicalResult mlir::parseSourceString(llvm::StringRef sourceStr, Block *block,
                                       MLIRContext *context,
                                       LocationAttr *sourceFileLoc) {
   auto memBuffer = MemoryBuffer::getMemBuffer(sourceStr);
   if (!memBuffer)
     return failure();
 
   SourceMgr sourceMgr;
   sourceMgr.AddNewSourceBuffer(std::move(memBuffer), SMLoc());
   return parseSourceFile(sourceMgr, block, context, sourceFileLoc);
 }

mlir/test/IR/invalid-unregistered.mlir

 // RUN: mlir-opt %s -split-input-file -verify-diagnostics
 
-// REQUIRES: noasserts
-
-// expected-error @below {{op created with unregistered dialect}}
+// expected-error @below {{operation being parsed with an unregistered dialect}}
 "unregistered_dialect.op"() : () -> ()
 
 // -----
 
 // expected-error @below {{attribute created with unregistered dialect}}
 #attr = #unregistered_dialect.attribute
 
 // -----
 
 // expected-error @below {{type created with unregistered dialect}}
 !type = type !unregistered_dialect.type