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.
Details
- Reviewers
rriddle nicolasvasilache bondhugula
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
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.