[llvm-branch-commits] [mlir] [draft] Dialect Conversion without Rollback (PR #93412)

[Matthias Springer via llvm-branch-commits]

matthias-springer wrote:

You are right, there is one thing that conversion patterns are no longer allowed to do. From the RFC:
```
One change is needed for existing conversion patterns: they must not modify the IR if they
return failure. (Same as ordinary rewrite patterns.) Most patterns should already be written in
such a way. for all other patterns, that should be a simple fix.
```

We already have assertions that check this in the greedy pattern rewrite driver (`ExpensiveChecks`); must be enabled explicitly because they are expensive. This prototype is reuses some of the greedy pattern rewrite driver, so we get these checks for free.

Swapping the order of the two conversion rewriters in the class hierarchy won't make a difference here: for patterns to work with the new driver, they must not make changes to the IR and then `return failure`. Also, if `ConversionRewriter` inherits from `OneShotConversionRewriter`, it won't be possible to use existing patterns with the new driver (unless we also change the name of `ConversionRewriter`->`LegacyConversionRewriter` and  `OneShotConversionRewriter`->`ConversionRewriter`.)

> I just fear that we will never be able to remove the inheritance if we build this in now.

I share some of that concern. But I find it important to build something that is backwards compatible (with existing conversion patterns); with a clear and simple migration path. Otherwise, nobody is going to use the new driver. (There are so many helpers like function op conversion patterns or structural SCF conversion patterns, that people will want to reuse.)

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