[Mlir-commits] [mlir] 05fb260 - [MLIR] Fix assert crash when an unregistered dialect op is encountered

[Uday Bondhugula]

Author: Uday Bondhugula
Date: 2021-10-14T15:43:53+05:30
New Revision: 05fb26062c32788c7b320937705fcac772ab21ff

URL: https://github.com/llvm/llvm-project/commit/05fb26062c32788c7b320937705fcac772ab21ff
DIFF: https://github.com/llvm/llvm-project/commit/05fb26062c32788c7b320937705fcac772ab21ff.diff

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

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

Differential Revision: https://reviews.llvm.org/D111628

Added: 
    

Modified: 
    mlir/include/mlir/IR/Operation.h
    mlir/include/mlir/IR/OperationSupport.h
    mlir/lib/IR/Operation.cpp
    mlir/lib/Parser/Parser.cpp
    mlir/test/IR/invalid-unregistered.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/IR/Operation.h b/mlir/include/mlir/IR/Operation.h
index d9ba5985aae65..198522885041a 100644
--- a/mlir/include/mlir/IR/Operation.h
+++ b/mlir/include/mlir/IR/Operation.h
@@ -99,7 +99,7 @@ class alignas(8) Operation final
   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.

diff  --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index 606c4cfbd6065..0ac793fdaa91d 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -383,8 +383,8 @@ class OperationName {
   /// 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;

diff  --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index e7cc0642120fd..8eb4c612dd612 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -180,7 +180,7 @@ Operation::Operation(Location location, OperationName name, unsigned numResults,
         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
 }
 

diff  --git a/mlir/lib/Parser/Parser.cpp b/mlir/lib/Parser/Parser.cpp
index 881892d5bc252..4d3f3b051a406 100644
--- a/mlir/lib/Parser/Parser.cpp
+++ b/mlir/lib/Parser/Parser.cpp
@@ -510,7 +510,7 @@ ParseResult OperationParser::finalize() {
       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");
     }
@@ -989,9 +989,18 @@ Operation *OperationParser::parseGenericOperation() {
   // 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)) {
-      result.name = OperationName(name, getContext());
+    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;
+      }
     }
   }
 
@@ -1689,8 +1698,8 @@ ParseResult OperationParser::parseRegionBody(
   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
@@ -1737,7 +1746,7 @@ ParseResult OperationParser::parseRegionBody(
   }
 
   // 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))
@@ -2071,13 +2080,13 @@ LogicalResult mlir::parseSourceFile(llvm::StringRef filename,
     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);
 }
 

diff  --git a/mlir/test/IR/invalid-unregistered.mlir b/mlir/test/IR/invalid-unregistered.mlir
index 30d4b7af88ed4..b059a0be33619 100644
--- a/mlir/test/IR/invalid-unregistered.mlir
+++ b/mlir/test/IR/invalid-unregistered.mlir
@@ -1,8 +1,6 @@
 // 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"() : () -> ()
 
 // -----