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

[Jacob Yu]

pingshiyu wrote:

@banach-space @joker-eph 

> We could change that, sure, but it's a discussion beyond Arith and MLIR, and more suitable for LLVM Discourse.

Yes, thanks for highlighting this. I do understand now that this is a deviation from what people are used to. I'm not very familiar with tests on the LLVM side.

Maybe we could work together on the integration testing guidelines proposal, as we seem to want changes in similar directions :) What other ways can I contact you to discuss this further? I am on Discord as well and my email is on my profile.

I have concerns about raising this with the whole LLVM community because it is quite a substantial scope expansion compared to what I had in mind: I thought integration tests would be useful for rewrites in MLIR, and this PR is initiating that for Arith. 

There's a focus on lowering patterns here as they are easier to test using integration tests (hard to control what canonicalization patterns get applied) - and the extensive use of lowerings is characteristic of MLIR rather than LLVM.

> we should select a formula that is known to be correct (surely there are some good references) and implement that.

I don't quite agree with this in the context of general testing approaches for MLIR. We can't expect to always have a well-known, or verified formula for all rewrite patterns. Even then, all rewrite patterns _are_ implementations of some formula - and implementations can have bugs, which would benefit from validation using integration tests (since bugs in the patterns cannot be detected by unit tests). 

(Note by validation I mean a unit-testing-like approach rather than an exhaustive test suite for all cases: AFAIK patterns don't have _any_ unit testing at all currently, and the question of whether they are correct, or correctly implement some formula, hinges almost entirely upon code reviews)

> how you make sure that the semantics in Haskell match MLIR (and whether there's scope for writing a test-suite). 

It's funny you mention this! xD The way I am making sure the semantics match is concurrently developing a fuzzer to generate MLIR programs (e.g. _lots_ of random integration tests), and comparing the semantics against the MLIR compilation results.

This very test suite in this PR actually comes from this process. Every test case here is a bug in either MLIR or in the Haskell semantics - a place where they disagreed at some point during development :)


Finally, for this particular series of PRs (integration tests for Arith), I think the following are the value add:
- it adds coverage that is beyond the scope of unit tests (testing the patterns themselves)
- previous and existing MLIR bugs do benefit from this additional coverage (https://github.com/llvm/llvm-project/issues/106519 is a case where a previous bug was "fixed", a unit test was added, but it became apparent when running the integration test that the bug was not completely fixed after all. that bug would have benefitted from integration tests!)

But we should probably return to this once we get some feedback from the wider MLIR community :)

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