[Mlir-commits] [mlir] [mlir][arith] Add more canonicalization and integration tests coverage (PR #92272)

[Jacob Yu]

pingshiyu wrote:

> Thanks for sharing these, @pingshiyu !
> 
> I'm always in favour of improving our test coverage, but if we intend to add much more then it would be good to have some plan.
> 
> Another question that I'd ask - how far do we go with testing? Do we want to exercise every possible combination? For example, looking at "test-int-trunc-ext.mlir" that you added, there are the following 3 cases:
> 
>     * `@extsiOnI1`
> 
>     * `@extuiOn1I1`
> 
>     * `@trunciI16ToI8`
> 
> 
> Why not more, i.e. other data types and operations? Once we consider all the possibilities, the potential set of all tests becomes really large. This can be a maintenance burden. Are we worried?
> 
> Also, if we are to add _many_ more tests, it would be good to establish some structure so that it's easy to identify cases that are covered and to find cases that are missing. Otherwise, we tend to duplicate tests, I've seen examples in the Vector dialect, not sure about Arith. This can usually be improved with comments and consistent test function naming.
> 
> > Are we happy with the regression test format right now (i.e. either as canonicalization tests, or as integration tests)? If so I'll add the rest of the tests into the PR too.
> 
> I find integration tests very helpful, but they tend to be expensive and hence hidden behind a CMake flag. Improving the coverage for Arith would be a welcome addition, but it would be good to have a strategy for selecting good test cases. To me, this is a very good reference:
> 
>     * https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Complex/CPU/correctness.mlir
> 
> 
> IIUC, there's a case for every operation and every Op is verified with a set of inputs. I know that @Lewuathe has recently contributed there - would you have any recommendation for good cases to test?

Definitely a good point on having the new tests be organised in a way that it's easy to identify gaps! 

The current tests don't cover _all_ cases, although I think it wouldn't be realistic to do so either - that would be a combinatorial explosion and is perhaps better explored using property-based testing :) For this PR, my aim is to just add all the tests that I have accrued from the interpreter exercise, and most of these were once a bug within the interpreter

In the most recent commit I've added the remainder of the tests as integration tests. I avoided the `canonicalization.mlir` tests, because like you pointed out, it is already rather large, unwieldy to go through, and there might well be duplicates.

I've assigned the tests a rough category for what they're testing, according to the file names added :)


https://github.com/llvm/llvm-project/pull/92272