[PATCH] D76903: Add a flag on the context to protect against creation of operations in unregistered dialects

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle marked an inline comment as done.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/IR/MLIRContext.h:53
+  /// Return true if we allow to create operation for unregistered dialects.
+  bool isAllowingUnregisteredDialect();
+
----------------
These sound weird, can we use the same wording as the options for operations/types in dialects? `allowsUnregisteredDialects()`/`allowUnregisteredDialects(bool allow)`?


================
Comment at: mlir/lib/Analysis/Verifier.cpp:209
     else
       it = dialectAllowsUnknownOps.try_emplace(dialectPrefix, true).first;
   }
----------------
Seems like you should put the check on this back-edge to avoid redundant lookups.


================
Comment at: mlir/lib/IR/MLIRContext.cpp:168
+
+  // In most cases, creating operation in unregistered dialect is not desired
+  // and indicate a misconfiguration of the compiler. This option enables to
----------------
Please use /// for comments.


================
Comment at: mlir/lib/Support/MlirOptMain.cpp:78
   MLIRContext context;
+  if (allowUnregisteredDialect)
+    context.setAllowingUnregisteredDialect();
----------------
Using the suggestion above would simplify this to just: `context.allowUnregisteredDialects(allowUnregisteredDialect)`.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:449
   if (isa<llvm::Instruction>(value)) {
-    OperationState state(UnknownLoc::get(context), "unknown");
+    OperationState state(UnknownLoc::get(context), "llvm.unknown");
     LLVMType type = processType(value->getType());
----------------
I actually would have expected this one to be fine, but it seems that OperationName::getDialect doesn't properly handle unregistered operations for the builtin dialect(no namespace). I think that is fine though.


================
Comment at: mlir/tools/mlir-translate/mlir-translate.cpp:49
+static llvm::cl::opt<bool> allowUnregisteredDialect(
+    "allow-unregistered-dialect",
+    llvm::cl::desc("Allow operation for unregistered dialects"),
----------------
This looks unused.


================
Comment at: mlir/unittests/IR/OperationSupportTest.cpp:18
 namespace {
 Operation *createOp(MLIRContext *context, bool resizableOperands,
                     ArrayRef<Value> operands = llvm::None,
----------------
Can you move this out of the namespace and mark as static while you are here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76903/new/

https://reviews.llvm.org/D76903