Defend early against operation created without a registered dialect

[mehdi_amini]

Summary

BEGIN_PUBLIC
Defend early against operation created without a registered dialect
END_PUBLIC

[rriddle]

mlir/include/mlir/IR/BuiltinOps.td

266 ↗

(On Diff #358518)

Accidentally change here?

mlir/lib/IR/Operation.cpp

180

Can we check this in the other place we check unregistered operations?

[jpienaar]

mlir/lib/IR/Operation.cpp

185

s/mlir-opt/MLIR opt tool used/ ? :) [no change needed, I was just thinking most folks will probably not be using mlir-opt vs their own one at this time]

[mehdi_amini]

mlir/include/mlir/IR/BuiltinOps.td

266 ↗

(On Diff #358518)

No: the parser creates this op to have Value for forward references.
We could change the parser to use block arguments instead as well

mlir/lib/IR/Operation.cpp

180

What do you mean?
There isn’t another ctor is there?

[rriddle]

mlir/include/mlir/IR/BuiltinOps.td

266 ↗

(On Diff #358518)

Ah, I see. Can we change the parser to use unrealized_conversion_cast or block arguments, as opposed to defining a new op in the builtin dialect?

mlir/lib/IR/Operation.cpp

180

Right, I was thinking of a check that I've discussed with someone before but it never got added. Can we move this check to OperationName instead?

[mehdi_amini]

mlir/include/mlir/IR/BuiltinOps.td

266 ↗

(On Diff #358518)

Yeah I'll do that.

mlir/lib/IR/Operation.cpp

180

We could, I'm slightly concerned that some folks may be creating an OperationName for the sake of trying to get an AbstractOperation.

[mehdi_amini]

• Use unrealized_cast in the parser
• Update error message

[mehdi_amini]

mlir/lib/IR/Operation.cpp

180

First hiccup with the OperationName approach: we create patterns this way!

Pattern::Pattern(StringRef rootName, PatternBenefit benefit,
                 MLIRContext *context, ArrayRef<StringRef> generatedNames)
    : Pattern(OperationName(rootName, context).getAsOpaquePointer(),
              RootKind::OperationName, generatedNames, benefit, context) {}

And we do it regardless of a dialect being loaded in the context, since we may create these on initialization and before the dialect is actually loaded.

185

Done! (and updated the other existing places with the same message while at it)

[mehdi_amini]

mlir/include/mlir/IR/BuiltinOps.td

266 ↗

(On Diff #358518)

I struggled to use BlockArguments in the parser, I ended up using unrealized_cast!

mlir/lib/IR/Operation.cpp

180

I struggled to use BlockArguments in the parser, I ended up using unrealized_cas

[rriddle]

mlir/lib/IR/Operation.cpp

180

That is part of the problem. The current way we have OperationName structured makes creating it before loading a dialect problematic: i.e. an OperationName created before is != to one created after loading.

[rriddle]

mlir/lib/IR/Operation.cpp

180

I'm fine with resolving that separately though. I think we should just switch to lazy loading like we do with dialect names in Identifiers. That would also lead to a performance improvement, by removing PointerUnion interaction.

[mehdi_amini]

mlir/lib/IR/Operation.cpp

180

OperationName created before is != to one created after loading.

Wait, but how do things work right now? It is likely common that we create patterns for ops that aren't registered yet, isn't it?

[rriddle]

mlir/lib/IR/Operation.cpp

180

If a pattern has an OperationName from before the op was registered, it won't match a registered operation AFAIK.

[mehdi_amini]

mlir/lib/IR/Operation.cpp

180

Surprising, matching an identifier should be enough, shouldn't we just keep an identifier pointer?

[stella.stamenova]

This change introduced a failure on Windows, but only when compiling with Visual Studio which is why it did not impact the buildbots (or really, almost any other uses cases, since most people use ninja). It came up as part of onnx-mlir merge with the latest llvm (https://github.com/onnx/onnx-mlir/pull/814) and I can confirm that I see the same failure locally.

It looks like when we build with Visual Studio, regardless of the state of LLVM_ENABLE_ASSERTIONS, enable_assertions is set to 0 because of this piece of code in AddLLVM.cmake (https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake):

if(LLVM_ENABLE_ASSERTIONS AND NOT MSVC_IDE)
  set(ENABLE_ASSERTIONS "1")
else()
  set(ENABLE_ASSERTIONS "0")
endif()

I think we have two options:

1. change the new test to be able to pass regardless of the state of assertions or change the logic in AddLLVM.cmake
2. change the logic in AddLLVM.cmake

(or, 3), I suppose, add a new config property which can track LLVM_ENABLE_ASSERTIONS regardless of MSVC_IDE, but I think that's a mistake).

I *think*, though I have not fully tested it yet, that changing the logic in AddLLVM.cmake will have no adverse effects on the builds/tests. It looks like this piece of code has been like that since 2011 and both the cmake setup for llvm and Visual Studio have changed sufficiently, that is is probably no longer necessary.

[mehdi_amini]

Thanks for tracking it down @stella.stamenova ; indeed the original commit is:

commit b46fdac4609df2613177813d43a124e3d9a8a306
Author: Andrew Trick <[e-mail]>
Date:   Tue Jun 28 16:32:01 2011

    cmake: Our MSVC build does not support config-time build mode.
    
    llvm-svn: 134008

There isn't much more info. If you remove the special case for MSVC_IDE and it works that'd be ideal.
Otherwise there is also the option to disable this test on windows for now.

[stella.stamenova]

In D105961#2975027, @mehdi_amini wrote:

Thanks for tracking it down @stella.stamenova ; indeed the original commit is:

commit b46fdac4609df2613177813d43a124e3d9a8a306
Author: Andrew Trick <[e-mail]>
Date:   Tue Jun 28 16:32:01 2011

    cmake: Our MSVC build does not support config-time build mode.
    
    llvm-svn: 134008

There isn't much more info. If you remove the special case for MSVC_IDE and it works that'd be ideal.
Otherwise there is also the option to disable this test on windows for now.

I've confirmed that we don't need the workaround for MSVC_IDE in the LLVM cmake file any more and it does not impact any of the other tests elsewhere in the project. @mehdi_amini : I can make the commit to AddLLVM.cmake or you could do it to fix the mlir test. Let me know.

[mehdi_amini]

The fix in AddLLVM.cmake seems best, I didn't quite get what we'd fix in the test itself?

[stella.stamenova]

In D105961#2987813, @mehdi_amini wrote:

The fix in AddLLVM.cmake seems best, I didn't quite get what we'd fix in the test itself?

I meant: I can make the change or you can, what would you prefer?

[mehdi_amini]

In D105961#2989791, @stella.stamenova wrote:

In D105961#2987813, @mehdi_amini wrote:

The fix in AddLLVM.cmake seems best, I didn't quite get what we'd fix in the test itself?

I meant: I can make the change or you can, what would you prefer?

Ah, please go ahead :)

mlir/lib/IR/BuiltinAttributes.cpp

LogicalResult OpaqueAttr::verify(function_ref<InFlightDiagnostic()> emitError,
   MLIRContext *context = dialect.getContext();
   if (!context->allowsUnregisteredDialects() &&
       !context->getLoadedDialect(dialect.strref())) {
     return emitError()
            << "#" << dialect << "<\"" << attrData << "\"> : " << type
            << " attribute created with unregistered dialect. If this is "
               "intended, please call allowUnregisteredDialects() on the "
               "MLIRContext, or use -allow-unregistered-dialect with "
-              "mlir-opt";
+              "the MLIR opt tool used";
   }
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // ElementsAttr
 //===----------------------------------------------------------------------===//

mlir/lib/IR/BuiltinTypes.cpp

LogicalResult OpaqueType::verify(function_ref<InFlightDiagnostic()> emitError,
   MLIRContext *context = dialect.getContext();
   if (!context->allowsUnregisteredDialects() &&
       !context->getLoadedDialect(dialect.strref())) {
     return emitError()
            << "`!" << dialect << "<\"" << typeData << "\">"
            << "` type created with unregistered dialect. If this is "
               "intended, please call allowUnregisteredDialects() on the "
               "MLIRContext, or use -allow-unregistered-dialect with "
-              "mlir-opt";
+              "the MLIR opt tool used";
   }
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // ShapedType
 //===----------------------------------------------------------------------===//

mlir/lib/IR/Operation.cpp

 
 Operation::Operation(Location location, OperationName name, unsigned numResults,
                      unsigned numSuccessors, unsigned numRegions,
                      DictionaryAttr attributes, bool hasOperandStorage)
     : location(location), numResults(numResults), numSuccs(numSuccessors),
       numRegions(numRegions), hasOperandStorage(hasOperandStorage), name(name),
       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");
+#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
   if (!use_empty()) {

mlir/lib/IR/Verifier.cpp

LogicalResult OperationVerifier::verifyOperation(
   // Otherwise, verify that the parent dialect allows un-registered operations.
   Dialect *dialect = opName.getDialect();
   if (!dialect) {
     if (!op.getContext()->allowsUnregisteredDialects()) {
       return op.emitOpError()
              << "created with unregistered dialect. If this is "
                 "intended, please call allowUnregisteredDialects() on the "
                 "MLIRContext, or use -allow-unregistered-dialect with "
-                "mlir-opt";
+                "the MLIR opt tool used";
     }
     return success();
   }
 
   if (!dialect->allowsUnknownOperations()) {
     return op.emitError("unregistered operation '")
            << op.getName() << "' found in dialect ('" << dialect->getNamespace()
            << "') that does not allow unknown operations";

mlir/lib/Parser/Parser.cpp

Value OperationParser::resolveSSAUse(SSAUseInfo useInfo, Type type) {
   // If the value has already been defined and this is an overly large result
   // number, diagnose that.
   if (entries[0].value && !isForwardRefPlaceholder(entries[0].value))
     return (emitError(useInfo.loc, "reference to invalid result number"),
             nullptr);
 
   // Otherwise, this is a forward reference.  Create a placeholder and remember
   // that we did so.
-  auto result = createForwardRefPlaceholder(useInfo.loc, type);
+  Value result = createForwardRefPlaceholder(useInfo.loc, type);
   entries[useInfo.number] = {result, useInfo.loc};
   return maybeRecordUse(result);
 }
 
 /// Parse an SSA use with an associated type.
 ///
 ///   ssa-use-and-type ::= ssa-use `:` type
 ParseResult OperationParser::parseSSADefOrUseAndType(

 /// Create and remember a new placeholder for a forward reference.
 Value OperationParser::createForwardRefPlaceholder(SMLoc loc, Type type) {
   // Forward references are always created as operations, because we just need
   // something with a def/use chain.
   //
   // We create these placeholders as having an empty name, which we know
   // cannot be created through normal user input, allowing us to distinguish
   // them.
-  auto name = OperationName("placeholder", getContext());
+  auto name = OperationName("unrealized_conversion_cast", getContext());
   auto *op = Operation::create(
       getEncodedSourceLocation(loc), name, type, /*operands=*/{},
       /*attributes=*/llvm::None, /*successors=*/{}, /*numRegions=*/0);
   forwardRefPlaceholders[op->getResult(0)] = loc;
   return op->getResult(0);
 }
 
 //===----------------------------------------------------------------------===//

mlir/test/CAPI/ir.c

static int constructAndTraverseIr(MlirContext ctx) {
   return 0;
 }
 
 /// Creates an operation with a region containing multiple blocks with
 /// operations and dumps it. The blocks and operations are inserted using
 /// block/operation-relative API and their final order is checked.
 static void buildWithInsertionsAndPrint(MlirContext ctx) {
   MlirLocation loc = mlirLocationUnknownGet(ctx);
+  mlirContextSetAllowUnregisteredDialects(ctx, true);
 
   MlirRegion owningRegion = mlirRegionCreate();
   MlirBlock nullBlock = mlirRegionGetFirstBlock(owningRegion);
   MlirOperationState state = mlirOperationStateGet(
       mlirStringRefCreateFromCString("insertion.order.test"), loc);
   mlirOperationStateAddOwnedRegions(&state, 1, &owningRegion);
   MlirOperation op = mlirOperationCreate(&state);
   MlirRegion region = mlirOperationGetRegion(op, 0);
static void buildWithInsertionsAndPrint(MlirContext ctx) {
   // Append operations to the rest of blocks to make them non-empty and thus
   // printable.
   mlirBlockAppendOwnedOperation(block2, op5);
   mlirBlockAppendOwnedOperation(block3, op6);
   mlirBlockAppendOwnedOperation(block4, op7);
 
   mlirOperationDump(op);
   mlirOperationDestroy(op);
+  mlirContextSetAllowUnregisteredDialects(ctx, false);
   // clang-format off
   // CHECK-LABEL:  "insertion.order.test"
   // CHECK:      ^{{.*}}(%{{.*}}: i1
   // CHECK:        "dummy.op1"
   // CHECK-NEXT:   "dummy.op2"
   // CHECK-NEXT:   "dummy.op3"
   // CHECK-NEXT:   "dummy.op4"
   // CHECK:      ^{{.*}}(%{{.*}}: i2

 }
 
 /// Tests operand APIs.
 int testOperands() {
   fprintf(stderr, "@testOperands\n");
   // CHECK-LABEL: @testOperands
 
   MlirContext ctx = mlirContextCreate();
+  mlirRegisterAllDialects(ctx);
+  mlirContextGetOrLoadDialect(ctx, mlirStringRefCreateFromCString("test"));
   MlirLocation loc = mlirLocationUnknownGet(ctx);
   MlirType indexType = mlirIndexTypeGet(ctx);
 
   // Create some constants to use as operands.
   MlirAttribute indexZeroLiteral =
       mlirAttributeParseGet(ctx, mlirStringRefCreateFromCString("0 : index"));
   MlirNamedAttribute indexZeroValueAttr = mlirNamedAttributeGet(
       mlirIdentifierGet(ctx, mlirStringRefCreateFromCString("value")),
int testOperands() {
   MlirOperationState constOneState = mlirOperationStateGet(
       mlirStringRefCreateFromCString("std.constant"), loc);
   mlirOperationStateAddResults(&constOneState, 1, &indexType);
   mlirOperationStateAddAttributes(&constOneState, 1, &indexOneValueAttr);
   MlirOperation constOne = mlirOperationCreate(&constOneState);
   MlirValue constOneValue = mlirOperationGetResult(constOne, 0);
 
   // Create the operation under test.
+  mlirContextSetAllowUnregisteredDialects(ctx, true);
   MlirOperationState opState =
       mlirOperationStateGet(mlirStringRefCreateFromCString("dummy.op"), loc);
   MlirValue initialOperands[] = {constZeroValue};
   mlirOperationStateAddOperands(&opState, 1, initialOperands);
   MlirOperation op = mlirOperationCreate(&opState);
 
   // Test operand APIs.
   intptr_t numOperands = mlirOperationGetNumOperands(op);
   fprintf(stderr, "Num Operands: %" PRIdPTR "\n", numOperands);
   // CHECK: Num Operands: 1
 
   MlirValue opOperand = mlirOperationGetOperand(op, 0);
   fprintf(stderr, "Original operand: ");
   mlirValuePrint(opOperand, printToStderr, NULL);
-  // CHECK: Original operand: {{.+}} {value = 0 : index}
+  // CHECK: Original operand: {{.+}} constant 0 : index
 
   mlirOperationSetOperand(op, 0, constOneValue);
   opOperand = mlirOperationGetOperand(op, 0);
   fprintf(stderr, "Updated operand: ");
   mlirValuePrint(opOperand, printToStderr, NULL);
-  // CHECK: Updated operand: {{.+}} {value = 1 : index}
+  // CHECK: Updated operand: {{.+}} constant 1 : index
 
   mlirOperationDestroy(op);
   mlirOperationDestroy(constZero);
   mlirOperationDestroy(constOne);
   mlirContextDestroy(ctx);
 
   return 0;
 }
 
 /// Tests clone APIs.
 int testClone() {
   fprintf(stderr, "@testClone\n");
   // CHECK-LABEL: @testClone
 
   MlirContext ctx = mlirContextCreate();
+  mlirRegisterAllDialects(ctx);
+  mlirContextGetOrLoadDialect(ctx, mlirStringRefCreateFromCString("std"));
   MlirLocation loc = mlirLocationUnknownGet(ctx);
   MlirType indexType = mlirIndexTypeGet(ctx);
   MlirStringRef valueStringRef =  mlirStringRefCreateFromCString("value");
 
   MlirAttribute indexZeroLiteral =
       mlirAttributeParseGet(ctx, mlirStringRefCreateFromCString("0 : index"));
   MlirNamedAttribute indexZeroValueAttr = mlirNamedAttributeGet(mlirIdentifierGet(ctx, valueStringRef), indexZeroLiteral);
   MlirOperationState constZeroState = mlirOperationStateGet(
       mlirStringRefCreateFromCString("std.constant"), loc);
   mlirOperationStateAddResults(&constZeroState, 1, &indexType);
   mlirOperationStateAddAttributes(&constZeroState, 1, &indexZeroValueAttr);
   MlirOperation constZero = mlirOperationCreate(&constZeroState);
 
   MlirAttribute indexOneLiteral =
       mlirAttributeParseGet(ctx, mlirStringRefCreateFromCString("1 : index"));
   MlirOperation constOne = mlirOperationClone(constZero);
   mlirOperationSetAttributeByName(constOne, valueStringRef, indexOneLiteral);
 
   mlirOperationPrint(constZero, printToStderr, NULL);
   mlirOperationPrint(constOne, printToStderr, NULL);
-  // CHECK: %0 = "std.constant"() {value = 0 : index} : () -> index
-  // CHECK: %0 = "std.constant"() {value = 1 : index} : () -> index
+  // CHECK: constant 0 : index
+  // CHECK: constant 1 : index
 
   return 0;
 }
 
 // Wraps a diagnostic into additional text we can match against.
 MlirLogicalResult errorHandler(MlirDiagnostic diagnostic, void *userData) {
   fprintf(stderr, "processing diagnostic (userData: %" PRIdPTR ") <<\n",
           (intptr_t)userData);

mlir/test/Dialect/PDL/invalid.mlir

pdl.pattern : benefit(1) {
   return
 }
 
 // -----
 
 // expected-error@below {{expected only `pdl` operations within the pattern body}}
 pdl.pattern : benefit(1) {
   // expected-note@below {{see non-`pdl` operation defined here}}
-  "foo.other_op"() : () -> ()
+  "test.foo.other_op"() : () -> ()
 
   %root = pdl.operation "foo.op"
   pdl.rewrite %root with "foo"
 }
 
 // -----
 
 pdl.pattern : benefit(1) {

mlir/test/IR/invalid-module-op.mlir

 // RUN: mlir-opt %s -split-input-file -verify-diagnostics
 
 // -----
 
 func @module_op() {
   // expected-error@+1 {{Operations with a 'SymbolTable' must have exactly one block}}
   module {
   ^bb1:
-    "module_terminator"() : () -> ()
+    "test.dummy"() : () -> ()
   ^bb2:
-    "module_terminator"() : () -> ()
+    "test.dummy"() : () -> ()
   }
   return
 }
 
 // -----
 
 func @module_op() {
   // expected-error@+1 {{region should have no arguments}}

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}}
 "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

mlir/test/lit.cfg.py

llvm_config.with_environment('PYTHONPATH', [
         # TODO: Don't reference the llvm_obj_root here: the invariant is that
         # the python/ must be at the same level of the lib directory
         # where libMLIR.so is installed. This is presently not optimal from a
         # project separation perspective and a discussion on how to better
         # segment MLIR libraries needs to happen. See also
         # lib/Bindings/Python/CMakeLists.txt for where this is set up.
         os.path.join(config.llvm_obj_root, 'python'),
     ], append_path=True)
+
+if config.enable_assertions:
+    config.available_features.add('asserts')
+else:
+    config.available_features.add('noasserts')

mlir/unittests/Pass/PassManagerTest.cpp

std::unique_ptr<Pass> clonePass() const override {
     return std::make_unique<InvalidPass>(
         *static_cast<const InvalidPass *>(this));
   }
 };
 } // anonymous namespace
 
 TEST(PassManagerTest, InvalidPass) {
   MLIRContext context;
+  context.allowUnregisteredDialects();
 
   // Create a module
   OwningModuleRef module(ModuleOp::create(UnknownLoc::get(&context)));
 
   // Add a single "invalid_op" operation
   OpBuilder builder(&module->getBodyRegion());
   OperationState state(UnknownLoc::get(&context), "invalid_op");
   builder.insert(Operation::create(state));