[mlir] Complety remove old `fold` API

[zero9178]

Summary

Last part of https://discourse.llvm.org/t/rfc-a-better-fold-api-using-more-generic-adaptors/67374

All active users that I am aware of have already switched. Any remaining users will be forced to adopt their code after this patch has landed, which I'd like to do on Wednesday. This is exactly 2 weeks after the PSA warning about it getting removed.

[mehdi_amini]

Thanks for following through!

[mehdi_amini]

mlir/include/mlir/IR/OpDefinition.h

1717

If we ignore ODS for a second, are we introduce the requirement that the T::FoldAdaptor exist on an Operation?
I'm not sure this should be a requirement though, if I write my C++ op outside of ODS I may not want to write an adaptor.

[zero9178]

mlir/include/mlir/IR/OpDefinition.h

1717

Somewhat. Only if the Op written in C++ implements fold as well, otherwise it should compile without issues given that the detector will just return false due to the substitution error. Some users could also somewhat hacky define their own FoldAdaptor that happens to be constructible with the same arguments as the real one.

Do you know of any occurrences this breaks in the wild that can't easily be migrated to ODS? I honestly did not think about users implementing Ops in C++ and thought this was a rather unsupported setup (even if just de-facto. E.g. some classes like LLVMConversionPatterns also assume existence of an Adaptor).

[zero9178]

mlir/include/mlir/IR/OpDefinition.h

1717

If C++ written Ops require a fold method I could revert the changes made here in the Op class to allow the old fold method to just be allowed from C++, while keeping the removal of the option from ODS to discourage its use as much as possible.
I'd rather avoid that if possible though.

[mehdi_amini]

mlir/include/mlir/IR/OpDefinition.h

1717

I honestly did not think about users implementing Ops in C++ and thought this was a rather unsupported setup

We really should keep support for C++ written op. We don't encourage it, we don't prioritize it, but it is supported and we shouldn't make it overly hard unnecessarily.
In particular, it seems to me that keeping the possibility of having the old fold() here is "low-cost" right? We just keep the overload detection and dispatch appropriately :)

(Simplifying ODS in the rest of the patch is an obvious general good thing to me)

[rriddle]

mlir/include/mlir/IR/OpDefinition.h

1717

I don't really understand why preserving support for C++ defined things (including attributes/types) is valuable. Why should we preserve that over pushing to use ODS? It's awkwardly restrictive, and we (at least I) don't really think much about those use cases at this point.

[zero9178]

mlir/include/mlir/IR/OpDefinition.h

1717

Fair enough. I'll draft up a patch real quick making the changes and adding proper regression tests.

[mehdi_amini]

mlir/include/mlir/IR/OpDefinition.h

1717

Why should we preserve that over pushing to use ODS?

I don't think this accurately framing what's happening here. Nothing it done "over pushing to use ODS". My take is:

1. ODS the main way to define operations, this is what we push people to use. This is what we focus our efforts in terms of ergonomic, documentation, support, etc.
2. Writing C++ directly is discouraged, but still supported as a best effort case. We should evaluate things in terms of cost/benefit ratio here: not implementing "crazy heroics" to improve the ergonomic of C++, but not "actively degrading it unnecessarily" either.

When we have to tradeoff between the two, we prioritize ODS (that is: we won't degrade the ODS experience over C++ concerns, including the maintenance of the ODS infrastructure which is complex enough as-is).

In the case at hand here, I can be convinced otherwise, I'd have to see what is means for an op to define manually the adaptor in practice.

[Mogball]

FYI there's still mention of this folder API thing in the MLIR docs

[zero9178]

In D144391#4148793, @Mogball wrote:

FYI there's still mention of this folder API thing in the MLIR docs

Thanks for the notice! Pushed a commit removing the section.