This is an archive of the discontinued LLVM Phabricator instance.

[mlir] NFC: move test/IR/core-ops.mlir to test/Dialect/StandardOps/roundtrip.mlir
AbandonedPublic

Authored by ftynse on Feb 25 2020, 5:31 AM.

Details

Summary

Early on, MLIR had a set of built-in "core" ops. These were moved to
what is now the Standard dialect, but the test remained under /IR and
with an outdated name. Move it to better reflect the current layout of
the code.

Diff Detail

Event Timeline

ftynse created this revision.Feb 25 2020, 5:31 AM
nicolasvasilache accepted this revision.Feb 25 2020, 6:12 AM
This revision is now accepted and ready to land.Feb 25 2020, 6:12 AM
bondhugula requested changes to this revision.Feb 25 2020, 7:15 AM
bondhugula added a subscriber: bondhugula.

roundtrip.mlir seems to be a generic name; ops.mlir?

This revision now requires changes to proceed.Feb 25 2020, 7:15 AM

roundtrip.mlir seems to be a generic name; ops.mlir?

I fail to see how "ops" is any less generic than "roundtrip". This test exercises essentially parsers and printers, that's what roundtrip is about.

IMO: roundtrip is more specific and descriptive about the test that "ops".

roundtrip.mlir seems to be a generic name; ops.mlir?

I fail to see how "ops" is any less generic than "roundtrip". This test exercises essentially parsers and printers, that's what roundtrip is about.

Hmm... a lot of the test cases do roundtripping (all the op parse/print?) and you don't have to prefix/suffix each with roundtrip. If the test case is all about standard dialect op parsing/printing(?), I felt ops.mlir was apt.

roundtrip.mlir seems to be a generic name; ops.mlir?

I fail to see how "ops" is any less generic than "roundtrip". This test exercises essentially parsers and printers, that's what roundtrip is about.

Hmm... a lot of the test cases do roundtripping (all the op parse/print?) and you don't have to prefix/suffix each with roundtrip. If the test case is all about standard dialect op parsing/printing(?), I felt ops.mlir was apt.

Most tests do _rely on_ roundtripping, only specific actually _test_ it. This specific file has header comments "Verify the printed output can be parsed." and "Verify the generic form can be parsed." and is indeed concerned only with the standard dialect.

bondhugula added a comment.EditedFeb 26 2020, 1:56 AM

roundtrip.mlir seems to be a generic name; ops.mlir?

I fail to see how "ops" is any less generic than "roundtrip". This test exercises essentially parsers and printers, that's what roundtrip is about.

Hmm... a lot of the test cases do roundtripping (all the op parse/print?) and you don't have to prefix/suffix each with roundtrip. If the test case is all about standard dialect op parsing/printing(?), I felt ops.mlir was apt.

Most tests do _rely on_ roundtripping, only specific actually _test_ it. This specific file has header comments "Verify the printed output can be parsed." and "Verify the generic form can be parsed." and is indeed concerned only with the standard dialect.

At the end of the day, if this is testing parsing and printing of standard ops, then ops.mlir appears to be the right name; means of testing (roundtripping) shouldn't be confused with what's being actually tested (which is the ops' syntax). On this note, when I see test/Dialect/*/*.mlir, I notice LinAlg and LLVMIR are the only two dialects that appear to use roundtrip.mlir instead of ops.mlir, and I feel those should be renamed as well. Please strive for consistency here because one is always trying a single specific name in their editor, etc. when looking for the right test cases irrespective of the Dialect.

$ find test -name ops.mlir
test/Dialect/AffineOps/ops.mlir
test/Dialect/GPU/ops.mlir
test/Dialect/VectorOps/ops.mlir
test/Dialect/OpenMP/ops.mlir
test/Dialect/SPIRV/ops.mlir
test/Dialect/Loops/ops.mlir

$ find test -name roundtrip.mlir
test/Dialect/LLVMIR/roundtrip.mlir
test/Dialect/Linalg/roundtrip.mlir

I would rather rename the other tests for consistency, suboptimal choices of the past should not define the policy. Tests in Linalg and LLVM are called roundtrip because they test roundtrip of types and arguments as well, which are, obviously, not ops.

I would rather rename the other tests for consistency, suboptimal choices of the past should not define the policy. Tests in Linalg and LLVM are called roundtrip because they test roundtrip of types and arguments as well, which are, obviously, not ops.

Types, arguments, attributes are all attached to / in conjunction with ops; I'd rather rename roundtrip.mlir -> op.mlir at those two inconsistent places. It's roundtrip.mlir that I find both less accurate and inconsistent, and this is adding to the inconsistency.

ftynse abandoned this revision.Feb 26 2020, 2:37 AM

I'll just keep everything in the current state then, feel free to send patches if you want.

I'll just keep everything in the current state then, feel free to send patches if you want.

There isn't a need to abandon this - it'll be good to know what others think about the names. Since we already have numerous directories under test/Dialect/* and there'll likely be more, it'll be good to come to a consensus on consistent names. For eg., Dialect/QuantOps/ has all kinds of 'parse' prefixed parse-*.mlir names. Having different styles is worse here than less accurate names. I'm happy of course to send a patch too once there is consensus.