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

Wed Feb 26 01:59:57 PST 2020

bondhugula added a comment.

In D75112#1892911 <https://reviews.llvm.org/D75112#1892911>, @ftynse wrote:

> In D75112#1891741 <https://reviews.llvm.org/D75112#1891741>, @bondhugula wrote:
>
> > In D75112#1891672 <https://reviews.llvm.org/D75112#1891672>, @ftynse wrote:
> >
> > > In D75112#1891307 <https://reviews.llvm.org/D75112#1891307>, @bondhugula wrote:
> > >
> > > > 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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75112/new/

https://reviews.llvm.org/D75112