This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Complety remove old `fold` API
ClosedPublic

Authored by zero9178 on Feb 20 2023, 6:08 AM.

Details

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.

Diff Detail

Event Timeline

zero9178 created this revision.Feb 20 2023, 6:08 AM
zero9178 requested review of this revision.Feb 20 2023, 6:08 AM
mehdi_amini accepted this revision.Feb 20 2023, 9:35 AM

Thanks for following through!

This revision is now accepted and ready to land.Feb 20 2023, 9:35 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Feb 22 2023, 12:56 PM
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 added inline comments.Feb 22 2023, 1:06 PM
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 added inline comments.Feb 22 2023, 1:17 PM
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 added inline comments.Feb 22 2023, 1:26 PM
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 added inline comments.Feb 22 2023, 1:29 PM
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 added inline comments.Feb 22 2023, 1:29 PM
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 added inline comments.Feb 22 2023, 1:36 PM
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.

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

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

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

mlir/lib/TableGen/Dialect.cpp