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

[Andrzej Warzyński]

banach-space wrote:

> In the meantime, tests like [this](https://github.com/llvm/llvm-project/pull/96974/files) are out-of-scope of what I expect to see here.

Thanks for this feedback, Mehdi!

I feel that we are missing some guidelines on what the expected scope of [integration tests](https://mlir.llvm.org/getting_started/TestingGuide/#integration-tests) should be. My high-level suggestion (based on this discussion) would be that integration tests should be reserved for cases much more complex than e.g. (extracted from [comparison.mlir](https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Arith/CPU/comparison.mlir) that Mehdi referred to uptrhead):
* `arith.cmpi` -> `llvm.icmp`.

Admittedly, that's not particularly involved. However, examples from [expand-ops.mlir](https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/Arith/expand-ops.mlir) (including `arith.floordivsi` that @pingshiyu mentioned) seem to be more complex and perhaps we should port those as e2e tests instead?

I am happy to work on a proposal, but will be OoO next two weeks (perhaps somebody else is free to volunteer?). In the meantime, we should probably pause this effort.

> A unit test that applies a pattern and checks the resulting IR doesn't check the pattern itself is correct. 

I don't follow this comment. Unit tests are there specifically to verify that the corresponding pattern is correct. In addition, every pattern is verified through code-review.

>  checking that "the translated code is equal to what we expect" does not give us confidence in correctness here

IMHO, in the example that you [referred to](https://github.com/llvm/llvm-project/pull/83248/files#diff-71577b98cdb92d5eb7583c3c3d5b9488f017f8f56dc6d05b7b940848f6e0bfc9), one "magic" formula for expanding `floordvisi` was replaced with another. IIUC, the only thing that was incorrect was the original formula. Perhaps the real issue here is that we are missing some formal spec for these things? 

> An integration test would resolve the above issues for lowering passes.

I am not convinced. I can see how an integration test would prevent us from hitting #83079, but we will never have an e2e for every possible scenario. That's just not feasible and against the grain of MLIR and LLVM. As for #83079, IMHO, we were (and still are) missing a formal spec for expanding `floordivsi`. An e2e would/will help, yes, but should be considered as complementary to a spec rather than a replacement.

While it sounds like we probably should revert some of the newly added e2e tests, I feel that overall we are making important progress in terms of test coverage for Arith. Thank you @pingshiyu !

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