[Mlir-commits] [mlir] [mlir][EmitC] Convert math::RoundEvenOp, SqrtOp and RsqrtOp (PR #190158)

[Andrzej Warzyński]

banach-space wrote:

Hi @aniragil 👋🏻  Thanks for reviewing!

>  just wondering about the combined testing: is it common practice to test "composed lowering"? doesn't it create redundant dependency on what the math-expanding pass does? 

Let me reply to that. We are interested in the Math-to-EmitC lowering/conversion for those specific Ops and the only way to verify that  (and to defend against regressions) is to use the expansion pass on top.

> The `mlir-runner` test `test-expand-math-approx.mlir` seems to do something similar for `llvm`, is this its `emitc` equivalent? 

Effectively, yes. However, our target is EmitC rather than LLVM, which also means that our options are more limited.

> There's also the `tosa` directory in emitc doing similar end-to-end lowering for `tosa` (not sure how I feel about that either).

That's an old test that I moved in https://github.com/llvm/llvm-project/pull/177339. 

Both "EmitC/tosa/ops.mlir" and "EmitC/math/ops.mlir‎"  added here are effectively "**integration**" tests and could be moved to e.g. "mlir/test/Integration" 🤷🏻 . However, those tests tend to be runnable, and the EmitC tests are not runnable. 

We could, however, convert to C++ and then compile to make sure that everything works as expected. Basically, instead of:
```bash
mlir-opt file.mlir | mlir-runner
```
(that's what we use for "runnable" tests) we could run this for EmitC:
```bash
mlir-opt file.mlir | mlir-translate -mlir-to-cpp | clang -Wall -Werror -fsyntax-only -c
```

I don't have a strong opinion, there aren't that many tests involved. I guess that it is also a matter of where EmitC is heading and what's the right testing strategy. From our perspective the priority is to make sure that we can lower from our TOSA models to EmitC and that nothing regresses. IMHO, transformation and conversion tests are insufficient for this.

WDYT?

-Andrzej

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