Page MenuHomePhabricator

Add a `-verify-roundtrip` option to `mlir-opt` intended to validate custom printer/parser completeness
AcceptedPublic

Authored by mehdi_amini on Oct 23 2020, 7:30 PM.

Diff Detail

Unit TestsFailed

TimeTest
30 mslinux > MLIR.Dialect/LLVMIR::roundtrip.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-opt /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/LLVMIR/roundtrip.mlir -verify-roundtrip | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/LLVMIR/roundtrip.mlir
50 mswindows > MLIR.Dialect/LLVMIR::roundtrip.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\mlir-opt.exe C:\ws\w16n2-1\llvm-project\premerge-checks\mlir\test\Dialect\LLVMIR\roundtrip.mlir -verify-roundtrip | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\mlir\test\Dialect\LLVMIR\roundtrip.mlir

Event Timeline

mehdi_amini created this revision.Oct 23 2020, 7:30 PM
Herald added a project: Restricted Project. · View Herald Transcript
mehdi_amini requested review of this revision.Oct 23 2020, 7:30 PM

I wonder if we should just make it the default?

ftynse accepted this revision.Oct 26 2020, 10:42 AM
ftynse added inline comments.
mlir/lib/Support/MlirOptMain.cpp
92

Do we want to still run passes in this case? I'd just return doVerifyRoundTrip if the flag is set.

This revision is now accepted and ready to land.Oct 26 2020, 10:42 AM

D90179 should fix the bug this found

rriddle accepted this revision.Oct 26 2020, 11:27 AM
rriddle added inline comments.
mlir/lib/Support/MlirOptMain.cpp
50

Can you emit an error here to give context(i.e. that these errors are from round trip) to the errors that may come out of parseSourceString?

64

nit: Add braces here, or move the comment.

65

nit: failed to round-trip resultant IR?

87

Why the newline here?

I wonder if we should just make it the default?

IMO, it depends on the user running mlir-opt. If I'm iterating locally, I generally wouldn't want these additional cost/complexity of running this. For tests that run automatically, that default is probably fine. I would strongly prefer that we keep mlir-opt defaults useful for users iterating locally and generally using mlir-opt as a development tool. For testing, could we just have another frontend(alias of mlir-opt?) that has better defaults for testing use cases? We've had conversations of sane defaults in the past, and it seems like the truly contentious ones always have this divide between local development and testing.

I wonder if we should just make it the default?

IMO, it depends on the user running mlir-opt. If I'm iterating locally, I generally wouldn't want these additional cost/complexity of running this. For tests that run automatically, that default is probably fine. I would strongly prefer that we keep mlir-opt defaults useful for users iterating locally and generally using mlir-opt as a development tool. For testing, could we just have another frontend(alias of mlir-opt?) that has better defaults for testing use cases? We've had conversations of sane defaults in the past, and it seems like the truly contentious ones always have this divide between local development and testing.

I'd also be pro not making it default from orthogonality point of view. Having all tests verifying printing/parsing vs having dedicated tests seems a wrong tradeoff. Yes having it default would catch cases not covered, but the fact that they aren't covered by unit tests is the problematic part that has to be addressed IMHO.

mlir/include/mlir/Support/MlirOptMain.h
47

I don't know at which point we refactor this into a options struct, but 6 bools in a row is quite easy to mess up.

mlir/lib/Support/MlirOptMain.cpp
40

Nit: this need not verify custom, depending on other flags passed (unless you set generic form to false on first print I believe the command line option for whether set or not takes preference) this just verifies printing and parsing.

77

Mmm, we now have 3 bools in a row here, thats a bit error prone

92

Or potentially rename to alsoVerifyRoundTrip to avoid confusion?

206

Roundtrip