-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Enables textual IR roundtripping through --verifyRoundtrip
#82946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enables textual IR roundtripping through --verifyRoundtrip
#82946
Conversation
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Matteo Franciolini (mfrancio) ChangesThis patch implements a mechanism to fail a bytecode roundtrip test only In addition to that, it fixes roundtripping to bytecode the Full diff: https://github.com/llvm/llvm-project/pull/82946.diff 1 Files Affected:
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,
|
39f3a09
to
0873e89
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
0873e89
to
6fe4a48
Compare
op->print(ostream, | ||
OpPrintingFlags().printGenericOpForm().enableDebugInfo()); | ||
// When testing a bytecode roundtrip, we don't want to report failure if the | ||
// textual roundtrip also fails. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
6fe4a48
to
a282ea9
Compare
--verifyRoundtrip
a282ea9
to
5f09fe3
Compare
There was a problem hiding this 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.
The patch enables roundtrip to textual file when running
--verifyRoundtrip
. The verification is successful if both textual and bytecode formats can roundtrip successfully.