Skip to content

Conversation

mfrancio
Copy link
Contributor

@mfrancio mfrancio commented Feb 26, 2024

The patch enables roundtrip to textual file when running --verifyRoundtrip. The verification is successful if both textual and bytecode formats can roundtrip successfully.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Feb 26, 2024
@mfrancio mfrancio requested a review from joker-eph February 26, 2024 00:49
@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2024

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Matteo Franciolini (mfrancio)

Changes

This patch implements a mechanism to fail a bytecode roundtrip test only
if the corresponding textual roundtrip is successful. Right now the test
assumes that printers/parsers are correctly implemented for the textual
format. This is not the case for some of the upstream dialects, for
example SPIRV.

In addition to that, it fixes roundtripping to bytecode the
IR/affine-map.mlir test, which relies on the parser to simplify some
of the affine maps. However, at the end of the parsing this
simplification is not complete: some expressions are further unique'd
after an additional roundtrip to file, either in text or bytecode
format.


Full diff: https://github.com/llvm/llvm-project/pull/82946.diff

1 Files Affected:

  • (modified) mlir/lib/Tools/mlir-opt/MlirOptMain.cpp (+57-26)
diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
index f01c7631decb77..36fe09d20b6e58 100644
--- a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
+++ b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
@@ -27,6 +27,7 @@
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/Location.h"
 #include "mlir/IR/MLIRContext.h"
+#include "mlir/IR/OwningOpRef.h"
 #include "mlir/Parser/Parser.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Pass/PassManager.h"
@@ -266,18 +267,62 @@ static LogicalResult doVerifyRoundTrip(Operation *op,
                                        const MlirOptMainConfig &config,
                                        bool useBytecode) {
   // We use a new context to avoid resource handle renaming issue in the diff.
+  auto initializeNewContext = [&](MLIRContext &newContext) -> LogicalResult {
+    OwningOpRef<Operation *> roundtripModule;
+    newContext.appendDialectRegistry(op->getContext()->getDialectRegistry());
+    if (op->getContext()->allowsUnregisteredDialects())
+      newContext.allowUnregisteredDialects();
+    StringRef irdlFile = config.getIrdlFile();
+    if (!irdlFile.empty() && failed(loadIRDLDialects(irdlFile, newContext)))
+      return failure();
+    return success();
+  };
   MLIRContext roundtripContext;
-  OwningOpRef<Operation *> roundtripModule;
-  roundtripContext.appendDialectRegistry(
-      op->getContext()->getDialectRegistry());
-  if (op->getContext()->allowsUnregisteredDialects())
-    roundtripContext.allowUnregisteredDialects();
-  StringRef irdlFile = config.getIrdlFile();
-  if (!irdlFile.empty() && failed(loadIRDLDialects(irdlFile, roundtripContext)))
+  if (failed(initializeNewContext(roundtripContext)))
     return failure();
 
+  auto parseMLIRString =
+      [&](const std::string &stringBuffer, MLIRContext &context) -> OwningOpRef<Operation *> {
+    FallbackAsmResourceMap fallbackResourceMap;
+    ParserConfig parseConfig(&context, /*verifyAfterParse=*/true,
+                             &fallbackResourceMap);
+    OwningOpRef<Operation *> roundtripModule =
+        parseSourceString<Operation *>(stringBuffer, parseConfig);
+    return roundtripModule;
+  };
+
+  // Print the operation to string. If we are going to verify the roundtrip to
+  // bytecode, make sure first that a roundtrip to text of the same IR is
+  // possible.
+  std::string reference;
+  {
+    llvm::raw_string_ostream ostream(reference);
+    op->print(ostream,
+              OpPrintingFlags().printGenericOpForm().enableDebugInfo());
+    // When testing a bytecode roundtrip, we don't want to report failure if the
+    // textual roundtrip also fails.
+    if (useBytecode) {
+      MLIRContext textualContext;
+      if (failed(initializeNewContext(textualContext)))
+        return failure();
+      OwningOpRef<Operation *> textualModule =
+          parseMLIRString(ostream.str(), textualContext);
+      // If we can't parse the string back, we can't guarantee that bytecode
+      // will be parsed correctly.
+      if (!textualModule)
+        return success();
+
+      // Clear the reference, and print the textual roundtrip.
+      reference.clear();
+      llvm::raw_string_ostream ostreamref(reference);
+      textualModule->print(
+          ostreamref, OpPrintingFlags().printGenericOpForm().enableDebugInfo());
+    }
+  }
+
   // Print a first time with custom format (or bytecode) and parse it back to
   // the roundtripModule.
+  std::string roundtrip;
   {
     std::string buffer;
     llvm::raw_string_ostream ostream(buffer);
@@ -291,25 +336,11 @@ static LogicalResult doVerifyRoundTrip(Operation *op,
       op->print(ostream,
                 OpPrintingFlags().printGenericOpForm(false).enableDebugInfo());
     }
-    FallbackAsmResourceMap fallbackResourceMap;
-    ParserConfig parseConfig(&roundtripContext, /*verifyAfterParse=*/true,
-                             &fallbackResourceMap);
-    roundtripModule =
-        parseSourceString<Operation *>(ostream.str(), parseConfig);
-    if (!roundtripModule) {
-      op->emitOpError()
-          << "failed to parse bytecode back, cannot verify round-trip.\n";
-      return failure();
-    }
-  }
-
-  // Print in the generic form for the reference module and the round-tripped
-  // one and compare the outputs.
-  std::string reference, roundtrip;
-  {
-    llvm::raw_string_ostream ostreamref(reference);
-    op->print(ostreamref,
-              OpPrintingFlags().printGenericOpForm().enableDebugInfo());
+    OwningOpRef<Operation *> roundtripModule =
+        parseMLIRString(ostream.str(), roundtripContext);
+    if (!roundtripModule)
+      return op->emitOpError()
+             << "failed to parse bytecode back, cannot verify round-trip.\n";
     llvm::raw_string_ostream ostreamrndtrip(roundtrip);
     roundtripModule.get()->print(
         ostreamrndtrip,

@mfrancio mfrancio force-pushed the dev/mfrancio/fixRoundtripVerification branch from 39f3a09 to 0873e89 Compare February 26, 2024 00:51
Copy link

github-actions bot commented Feb 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@mfrancio mfrancio force-pushed the dev/mfrancio/fixRoundtripVerification branch from 0873e89 to 6fe4a48 Compare February 26, 2024 01:02
op->print(ostream,
OpPrintingFlags().printGenericOpForm().enableDebugInfo());
// When testing a bytecode roundtrip, we don't want to report failure if the
// textual roundtrip also fails.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just do both test, and report the failure independently.

That is we exit with an error if either of them fails, but we always print the result for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! it seems you had all already setup for this. Updated (as well as the scope of the patch).

@mfrancio mfrancio force-pushed the dev/mfrancio/fixRoundtripVerification branch from 6fe4a48 to a282ea9 Compare February 26, 2024 02:15
@mfrancio mfrancio changed the title Strenghten bytecode roundtrip test Enables textual IR roundtripping through --verifyRoundtrip Feb 26, 2024
@mfrancio mfrancio force-pushed the dev/mfrancio/fixRoundtripVerification branch from a282ea9 to 5f09fe3 Compare February 26, 2024 02:32
Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG!
Please just add a description before merging.

@mfrancio mfrancio merged commit ce4da0c into llvm:main Feb 26, 2024
@mfrancio mfrancio deleted the dev/mfrancio/fixRoundtripVerification branch February 26, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants