[mlir] Making verification after parsing optional

[rriddle]

Summary

This is very useful when you want to parse IR even if
its invalid (e.g. bytecode). It's also useful if you don't
want to pay the cost of verification in certain situations.

[lattner]

Nice, thank you for the test

[bondhugula]

mlir/unittests/Parser/ParserTest.cpp

2

This is a header file's header. Drop "- C++ -".

[bzcheeseman]

Thanks river :)

[GleasonK]

Just noticed this config existed - will be very useful! Thanks!

mlir/include/mlir/IR/AsmState.h

 // ParserConfig
 //===----------------------------------------------------------------------===//
 
 /// This class represents a configuration for the MLIR assembly parser. It
 /// contains all of the necessary state to parse a MLIR source file.
 class ParserConfig {
 public:
   /// Construct a parser configuration with the given context.
+  /// `verifyAfterParse` indicates if the IR should be verified after parsing.
   /// `fallbackResourceMap` is an optional fallback handler that can be used to
   /// parse external resources not explicitly handled by another parser.
-  ParserConfig(MLIRContext *context,
+  ParserConfig(MLIRContext *context, bool verifyAfterParse = true,
                FallbackAsmResourceMap *fallbackResourceMap = nullptr)
-      : context(context), fallbackResourceMap(fallbackResourceMap) {
+      : context(context), verifyAfterParse(verifyAfterParse),
+        fallbackResourceMap(fallbackResourceMap) {
     assert(context && "expected valid MLIR context");
   }
 
   /// Return the MLIRContext to be used when parsing.
   MLIRContext *getContext() const { return context; }
 
+  /// Returns if the parser should verify the IR after parsing.
+  bool shouldVerifyAfterParse() const { return verifyAfterParse; }
+
   /// Return the resource parser registered to the given name, or nullptr if no
   /// parser with `name` is registered.
   AsmResourceParser *getResourceParser(StringRef name) const {
     auto it = resourceParsers.find(name);
     if (it != resourceParsers.end())
       return it->second.get();
     if (fallbackResourceMap)
       return &fallbackResourceMap->getParserFor(name);
std::enable_if_t<std::is_convertible<
       CallableT, function_ref<LogicalResult(AsmParsedResourceEntry &)>>::value>
   attachResourceParser(StringRef name, CallableT &&parserFn) {
     attachResourceParser(AsmResourceParser::fromCallable(
         name, std::forward<CallableT>(parserFn)));
   }
 
 private:
   MLIRContext *context;
+  bool verifyAfterParse;
   DenseMap<StringRef, std::unique_ptr<AsmResourceParser>> resourceParsers;
   FallbackAsmResourceMap *fallbackResourceMap;
 };
 
 //===----------------------------------------------------------------------===//
 // AsmState
 //===----------------------------------------------------------------------===//
 

mlir/lib/AsmParser/Parser.cpp

ParseResult OperationParser::finalize() {
   if (walkRes.wasInterrupted())
     return failure();
 
   // Pop the top level name scope.
   if (failed(popSSANameScope()))
     return failure();
 
   // Verify that the parsed operations are valid.
-  if (failed(verify(topLevelOp)))
+  if (state.config.shouldVerifyAfterParse() && failed(verify(topLevelOp)))
     return failure();
 
   // If we are populating the parser state, finalize the top-level operation.
   if (state.asmState)
     state.asmState->finalize(topLevelOp);
   return success();
 }
 

mlir/lib/Bytecode/Reader/BytecodeReader.cpp

while (!regionStack.empty())
     if (failed(parseRegions(reader, regionStack, regionStack.back())))
       return failure();
   if (!forwardRefOps.empty()) {
     return reader.emitError(
         "not all forward unresolved forward operand references");
   }
 
   // Verify that the parsed operations are valid.
-  if (failed(verify(*moduleOp)))
+  if (config.shouldVerifyAfterParse() && failed(verify(*moduleOp)))
     return failure();
 
   // Splice the parsed operations over to the provided top-level block.
   auto &parsedOps = moduleOp->getBody()->getOperations();
   auto &destOps = block->getOperations();
   destOps.splice(destOps.empty() ? destOps.end() : std::prev(destOps.end()),
                  parsedOps, parsedOps.begin(), parsedOps.end());
   return success();

mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp

MLIRDocument::MLIRDocument(MLIRContext &context, const lsp::URIForFile &uri,
 
   // Try to parsed the given IR string.
   auto memBuffer = llvm::MemoryBuffer::getMemBufferCopy(contents, uri.file());
   if (!memBuffer) {
     lsp::Logger::error("Failed to create memory buffer for file", uri.file());
     return;
   }
 
-  ParserConfig config(&context, &fallbackResourceMap);
+  ParserConfig config(&context, /*verifyAfterParse=*/true,
+                      &fallbackResourceMap);
   sourceMgr.AddNewSourceBuffer(std::move(memBuffer), SMLoc());
   if (failed(parseAsmSourceFile(sourceMgr, &parsedIR, config, &asmState))) {
     // If parsing failed, clear out any of the current state.
     parsedIR.clear();
     asmState = AsmParserState();
     fallbackResourceMap = FallbackAsmResourceMap();
     return;
   }
lsp::MLIRServer::convertFromBytecode(const URIForFile &uri) {
   ScopedDiagnosticHandler diagHandler(
       &tempContext,
       [&](mlir::Diagnostic &diag) { errorMsg += diag.str() + "\n"; });
 
   // Handling for external resources, which we want to propagate up to the user.
   FallbackAsmResourceMap fallbackResourceMap;
 
   // Setup the parser config.
-  ParserConfig parserConfig(&tempContext, &fallbackResourceMap);
+  ParserConfig parserConfig(&tempContext, /*verifyAfterParse=*/true,
+                            &fallbackResourceMap);
 
   // Try to parse the given source file.
   Block parsedBlock;
   if (failed(parseSourceFile(uri.file(), &parsedBlock, parserConfig))) {
     return llvm::make_error<lsp::LSPError>(
         "failed to parse bytecode source file: " + errorMsg,
         lsp::ErrorCode::RequestFailed);
   }

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp

static LogicalResult performActions(raw_ostream &os, bool verifyDiagnostics,
   context->disableMultithreading();
 
   // Prepare the parser config, and attach any useful/necessary resource
   // handlers. Unhandled external resources are treated as passthrough, i.e.
   // they are not processed and will be emitted directly to the output
   // untouched.
   PassReproducerOptions reproOptions;
   FallbackAsmResourceMap fallbackResourceMap;
-  ParserConfig config(context, &fallbackResourceMap);
+  ParserConfig config(context, /*verifyAfterParse=*/true, &fallbackResourceMap);
   reproOptions.attachResourceParser(config);
 
   // Parse the input file and reset the context threading state.
   TimingScope parserTiming = timing.nest("Parser");
   OwningOpRef<Operation *> op =
       parseSourceFileForTool(sourceMgr, config, implicitModule);
   context->enableMultithreading(wasThreadingEnabled);
   if (!op)

mlir/unittests/Parser/CMakeLists.txt

 add_mlir_unittest(MLIRParserTests
+  ParserTest.cpp
   ResourceTest.cpp
 
   DEPENDS
   MLIRTestInterfaceIncGen
 )
 target_include_directories(MLIRParserTests PRIVATE "${MLIR_BINARY_DIR}/test/lib/Dialect/Test")
 
 target_link_libraries(MLIRParserTests PRIVATE
   MLIRIR
   MLIRParser
   MLIRTestDialect
 )

mlir/unittests/Parser/ParserTest.cpp

+//===- ParserTest.cpp -----------------------------------------------------===//
+//
+// This file is licensed 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/Parser/Parser.h"
+#include "mlir/IR/BuiltinOps.h"
+#include "mlir/IR/Verifier.h"
+
+#include "gmock/gmock.h"
+
+using namespace mlir;
+
+namespace {
+TEST(MLIRParser, ParseInvalidIR) {
+  std::string moduleStr = R"mlir(
+    module attributes {bad} {}
+  )mlir";
+
+  MLIRContext context;
+  ParserConfig config(&context, /*verifyAfterParse=*/false);
+
+  // Check that we properly parse the op, but it fails the verifier.
+  OwningOpRef<ModuleOp> module = parseSourceString<ModuleOp>(moduleStr, config);
+  ASSERT_TRUE(module);
+  ASSERT_TRUE(failed(verify(*module)));
+}
+} // namespace