[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.

mlir/include/mlir/IR/DialectBase.td

 class CppDeprecated<string reason> {
   string odsCppDeprecated = reason;
 }
 
 //===----------------------------------------------------------------------===//
 // Dialect definitions
 //===----------------------------------------------------------------------===//
 
-class EmitFolderBase;
-// Generate 'fold' method with 'ArrayRef<Attribute>' parameter.
-// New code should prefer using 'kEmitFoldAdaptorFolder' and
-// consider 'kEmitRawAttributesFolder' deprecated and to be
-// removed in the future.
-def kEmitRawAttributesFolder : EmitFolderBase, Deprecated<
-  "'useFoldAPI' of 'kEmitRawAttributesFolder' (default) has been deprecated "
-  # "and is pending removal. Please switch to 'kEmitFoldAdaptorFolder'. See "
-  # "https://discourse.llvm.org/t/psa-new-improved-fold-method-signature-has-landed-please-update-your-downstream-projects/67618"
-> {}
-// Generate 'fold' method with 'FoldAdaptor' parameter.
-def kEmitFoldAdaptorFolder : EmitFolderBase {}
-
 class Dialect {
   // The name of the dialect.
   string name = ?;
 
   // Short summary of the dialect.
   string summary = ?;
 
   // The description of the dialect.
class Dialect {
   // individual type directly.
   bit useDefaultTypePrinterParser = 0;
 
   // If this dialect overrides the hook for canonicalization patterns.
   bit hasCanonicalizer = 0;
 
   // If this dialect can be extended at runtime with new operations or types.
   bit isExtensible = 0;
-
-  // Fold API to use for operations in this dialect.
-  EmitFolderBase useFoldAPI = kEmitFoldAdaptorFolder;
 }
 
 #endif // DIALECTBASE_TD

mlir/include/mlir/IR/OpDefinition.h

static void attachInterface(MLIRContext &context) {
     (checkInterfaceTarget<Models>(), ...);
     info->attachInterface<Models...>();
   }
 
 private:
   /// Trait to check if T provides a 'fold' method for a single result op.
   template <typename T, typename... Args>
   using has_single_result_fold_t =
-      decltype(std::declval<T>().fold(std::declval<ArrayRef<Attribute>>()));
+      decltype(std::declval<T>().fold(std::declval<typename T::FoldAdaptor>()));
   template <typename T>
   constexpr static bool has_single_result_fold_v =
       llvm::is_detected<has_single_result_fold_t, T>::value;
   /// Trait to check if T provides a general 'fold' method.
   template <typename T, typename... Args>
   using has_fold_t = decltype(std::declval<T>().fold(
-      std::declval<ArrayRef<Attribute>>(),
+      std::declval<typename T::FoldAdaptor>(),
       std::declval<SmallVectorImpl<OpFoldResult> &>()));
   template <typename T>
   constexpr static bool has_fold_v = llvm::is_detected<has_fold_t, T>::value;
-  /// Trait to check if T provides a 'fold' method with a FoldAdaptor for a
-  /// single result op.
-  template <typename T, typename... Args>
-  using has_fold_adaptor_single_result_fold_t =
-      decltype(std::declval<T>().fold(std::declval<typename T::FoldAdaptor>()));
-  template <class T>
-  constexpr static bool has_fold_adaptor_single_result_v =
-      llvm::is_detected<has_fold_adaptor_single_result_fold_t, T>::value;
-  /// Trait to check if T provides a general 'fold' method with a FoldAdaptor.
-  template <typename T, typename... Args>
-  using has_fold_adaptor_fold_t = decltype(std::declval<T>().fold(
-      std::declval<typename T::FoldAdaptor>(),
-      std::declval<SmallVectorImpl<OpFoldResult> &>()));
-  template <class T>
-  constexpr static bool has_fold_adaptor_v =
-      llvm::is_detected<has_fold_adaptor_fold_t, T>::value;
 
   /// Trait to check if T provides a 'print' method.
   template <typename T, typename... Args>
   using has_print =
       decltype(std::declval<T>().print(std::declval<OpAsmPrinter &>()));
   template <typename T>
   using detect_has_print = llvm::is_detected<has_print, T>;
 
private:
 
   /// Return the internal implementations of each of the OperationName
   /// hooks.
   /// Implementation of `FoldHookFn` OperationName hook.
   static OperationName::FoldHookFn getFoldHookFn() {
     // If the operation is single result and defines a `fold` method.
     if constexpr (llvm::is_one_of<OpTrait::OneResult<ConcreteType>,
                                   Traits<ConcreteType>...>::value &&
-                  (has_single_result_fold_v<ConcreteType> ||
-                   has_fold_adaptor_single_result_v<ConcreteType>))
+                  has_single_result_fold_v<ConcreteType>)
       return [](Operation *op, ArrayRef<Attribute> operands,
                 SmallVectorImpl<OpFoldResult> &results) {
         return foldSingleResultHook<ConcreteType>(op, operands, results);
       };
     // The operation is not single result and defines a `fold` method.
-    if constexpr (has_fold_v<ConcreteType> || has_fold_adaptor_v<ConcreteType>)
+    if constexpr (has_fold_v<ConcreteType>)
       return [](Operation *op, ArrayRef<Attribute> operands,
                 SmallVectorImpl<OpFoldResult> &results) {
         return foldHook<ConcreteType>(op, operands, results);
       };
     // The operation does not define a `fold` method.
     return [](Operation *op, ArrayRef<Attribute> operands,
               SmallVectorImpl<OpFoldResult> &results) {
       // In this case, we only need to fold the traits of the operation.
       return op_definition_impl::foldTraits<Traits<ConcreteType>...>(
           op, operands, results);
     };
   }
   /// Return the result of folding a single result operation that defines a
   /// `fold` method.
   template <typename ConcreteOpT>
   static LogicalResult
   foldSingleResultHook(Operation *op, ArrayRef<Attribute> operands,
                        SmallVectorImpl<OpFoldResult> &results) {
-    OpFoldResult result;
-    if constexpr (has_fold_adaptor_single_result_v<ConcreteOpT>)
-      result = cast<ConcreteOpT>(op).fold(typename ConcreteOpT::FoldAdaptor(
+    OpFoldResult result =
+        cast<ConcreteOpT>(op).fold(typename ConcreteOpT::FoldAdaptor(
             operands, op->getAttrDictionary(), op->getRegions()));
-    else
-      result = cast<ConcreteOpT>(op).fold(operands);
 
     // If the fold failed or was in-place, try to fold the traits of the
     // operation.
     if (!result || result.template dyn_cast<Value>() == op->getResult(0)) {
       if (succeeded(op_definition_impl::foldTraits<Traits<ConcreteType>...>(
               op, operands, results)))
         return success();
       return success(static_cast<bool>(result));
     }
     results.push_back(result);
     return success();
   }
   /// Return the result of folding an operation that defines a `fold` method.
   template <typename ConcreteOpT>
   static LogicalResult foldHook(Operation *op, ArrayRef<Attribute> operands,
                                 SmallVectorImpl<OpFoldResult> &results) {
-    auto result = LogicalResult::failure();
-    if constexpr (has_fold_adaptor_v<ConcreteOpT>) {
-      result = cast<ConcreteOpT>(op).fold(
-          typename ConcreteOpT::FoldAdaptor(operands, op->getAttrDictionary(),
-                                            op->getRegions()),
-          results);
-    } else {
-      result = cast<ConcreteOpT>(op).fold(operands, results);
-    }
+    LogicalResult result = cast<ConcreteOpT>(op).fold(
 
     // If the fold failed or was in-place, try to fold the traits of the
     // operation.
     if (failed(result) || results.empty()) {
       if (succeeded(op_definition_impl::foldTraits<Traits<ConcreteType>...>(
               op, operands, results)))
         return success();
     }

mlir/include/mlir/TableGen/Dialect.h

public:
   /// Returns true if this dialect should generate the default dispatch for
   /// type printing/parsing.
   bool useDefaultTypePrinterParser() const;
 
   /// Returns true if this dialect can be extended at runtime with new
   /// operations or types.
   bool isExtensible() const;
 
-  enum class FolderAPI {
-    RawAttributes = 0, /// fold method with ArrayRef<Attribute>.
-    FolderAdaptor = 1, /// fold method with the operation's FoldAdaptor.
-  };
-
-  /// Returns the folder API that should be emitted for operations in this
-  /// dialect.
-  FolderAPI getFolderAPI() const;
-
   // Returns whether two dialects are equal by checking the equality of the
   // underlying record.
   bool operator==(const Dialect &other) const;
 
   bool operator!=(const Dialect &other) const { return !(*this == other); }
 
   // Compares two dialects by comparing the names of the dialects.
   bool operator<(const Dialect &other) const;

mlir/lib/TableGen/Dialect.cpp

 bool Dialect::useDefaultTypePrinterParser() const {
   return def->getValueAsBit("useDefaultTypePrinterParser");
 }
 
 bool Dialect::isExtensible() const {
   return def->getValueAsBit("isExtensible");
 }
 
-Dialect::FolderAPI Dialect::getFolderAPI() const {
-  llvm::Record *value = def->getValueAsDef("useFoldAPI");
-  auto converted =
-      llvm::StringSwitch<std::optional<Dialect::FolderAPI>>(value->getName())
-          .Case("kEmitRawAttributesFolder", FolderAPI::RawAttributes)
-          .Case("kEmitFoldAdaptorFolder", FolderAPI::FolderAdaptor)
-          .Default(std::nullopt);
-
-  if (!converted)
-    llvm::PrintFatalError(def->getLoc(),
-                          "Invalid value for dialect field `useFoldAPI`");
-
-  return *converted;
-}
-
 bool Dialect::operator==(const Dialect &other) const {
   return def == other.def;
 }
 
 bool Dialect::operator<(const Dialect &other) const {
   return getName() < other.getName();
 }

mlir/test/mlir-tblgen/has-fold-invalid-values.td

-// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include %s 2>&1 | FileCheck %s
-
-include "mlir/IR/OpBase.td"
-
-def Bad : EmitFolderBase;
-
-def Test_Dialect : Dialect {
-  let name = "test";
-  let cppNamespace = "NS";
-  let useFoldAPI = Bad;
-}
-
-def InvalidValue_Op : Op<Test_Dialect, "invalid_op"> {
-  let hasFolder = 1;
-}
-
-// CHECK: Invalid value for dialect field `useFoldAPI`

mlir/test/mlir-tblgen/op-decl-and-defs.td

 // RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s | FileCheck %s
 // RUN: mlir-tblgen -gen-op-decls -op-include-regex="test.a_op" -I %S/../../include %s | FileCheck  %s --check-prefix=REDUCE_INC
 // RUN: mlir-tblgen -gen-op-decls -op-exclude-regex="test.a_op" -I %S/../../include %s | FileCheck  %s --check-prefix=REDUCE_EXC
 
 // RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s --check-prefix=DEFS
 
 include "mlir/IR/AttrTypeBase.td"
 include "mlir/IR/OpBase.td"
 include "mlir/Interfaces/InferTypeOpInterface.td"
 include "mlir/Interfaces/SideEffectInterfaces.td"
 
 def Test_Dialect : Dialect {
   let name = "test";
   let cppNamespace = "NS";
-  let useFoldAPI = kEmitRawAttributesFolder;
 }
 class NS_Op<string mnemonic, list<Trait> traits> :
     Op<Test_Dialect, mnemonic, traits>;
 
 // IsolatedFromAbove trait is included twice to ensure it gets uniqued during
 // emission.
 def NS_AOp : NS_Op<"a_op", [IsolatedFromAbove, IsolatedFromAbove]> {
   let arguments = (ins

 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value a, ::mlir::ValueRange b, ::mlir::IntegerAttr attr1, /*optional*/::mlir::FloatAttr some_attr2, unsigned someRegionsCount);
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type r, ::mlir::TypeRange s, ::mlir::Value a, ::mlir::ValueRange b, uint32_t attr1, /*optional*/::mlir::FloatAttr some_attr2, unsigned someRegionsCount)
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value a, ::mlir::ValueRange b, uint32_t attr1, /*optional*/::mlir::FloatAttr some_attr2, unsigned someRegionsCount);
 // CHECK:   static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes, unsigned numRegions)
 // CHECK:   static ::mlir::ParseResult parse(::mlir::OpAsmParser &parser, ::mlir::OperationState &result);
 // CHECK:   void print(::mlir::OpAsmPrinter &p);
 // CHECK:   ::mlir::LogicalResult verifyInvariants();
 // CHECK:   static void getCanonicalizationPatterns(::mlir::RewritePatternSet &results, ::mlir::MLIRContext *context);
-// CHECK:   ::mlir::LogicalResult fold(::llvm::ArrayRef<::mlir::Attribute> operands, ::llvm::SmallVectorImpl<::mlir::OpFoldResult> &results);
+// CHECK:   ::mlir::LogicalResult fold(FoldAdaptor adaptor, ::llvm::SmallVectorImpl<::mlir::OpFoldResult> &results);
 // CHECK:   // Display a graph for debugging purposes.
 // CHECK:   void displayGraph();
 // CHECK: };
 
 // DEFS-LABEL: NS::AOp definitions
 
 // DEFS: AOpGenericAdaptorBase::AOpGenericAdaptorBase(::mlir::DictionaryAttr attrs, ::mlir::RegionRange regions) : odsAttrs(attrs), odsRegions(regions)
 // DEFS: ::mlir::RegionRange AOpGenericAdaptorBase::getSomeRegions()

 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Value a, ::mlir::Value b, ::mlir::IntegerAttr attr1);
 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value a, ::mlir::Value b, ::mlir::IntegerAttr attr1);
 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type r, ::mlir::Value a, ::mlir::Value b, uint32_t attr1);
 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Value a, ::mlir::Value b, uint32_t attr1);
 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value a, ::mlir::Value b, uint32_t attr1);
 // CHECK: static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
 
-def TestWithNewFold_Dialect : Dialect {
-  let name = "test";
-  let cppNamespace = "::mlir::testWithFold";
-}
-
-def NS_MOp : Op<TestWithNewFold_Dialect, "op_with_single_result_and_fold_adaptor_fold", []> {
+def NS_MOp : NS_Op<"op_with_single_result_and_fold_adaptor_fold", []> {
   let results = (outs AnyType:$res);
 
   let hasFolder = 1;
 }
 
 // CHECK-LABEL: class MOp :
 // CHECK: ::mlir::OpFoldResult fold(FoldAdaptor adaptor);
 
-def NS_NOp : Op<TestWithNewFold_Dialect, "op_with_multiple_results_and_fold_adaptor_fold", []> {
-  let results = (outs AnyType:$res1, AnyType:$res2);
-
-  let hasFolder = 1;
-}
-
-// CHECK-LABEL: class NOp :
-// CHECK: ::mlir::LogicalResult fold(FoldAdaptor adaptor, ::llvm::SmallVectorImpl<::mlir::OpFoldResult> &results);
-
 // Test that type defs have the proper namespaces when used as a constraint.
 // ---
 
 def Test_Dialect2 : Dialect {
   let name = "test";
   let cppNamespace = "::mlir::dialect2";
 }
 def TestDialect2Type : TypeDef<Test_Dialect2, "Dialect2Type">;

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

if (hasBody) {
     method->body() << "  results.add(canonicalize);\n";
   }
 }
 
 void OpEmitter::genFolderDecls() {
   if (!op.hasFolder())
     return;
 
-  Dialect::FolderAPI folderApi = op.getDialect().getFolderAPI();
   SmallVector<MethodParameter> paramList;
-  if (folderApi == Dialect::FolderAPI::RawAttributes)
-    paramList.emplace_back("::llvm::ArrayRef<::mlir::Attribute>", "operands");
-  else
-    paramList.emplace_back("FoldAdaptor", "adaptor");
 
   StringRef retType;
   bool hasSingleResult =
       op.getNumResults() == 1 && op.getNumVariableLengthResults() == 0;
   if (hasSingleResult) {
     retType = "::mlir::OpFoldResult";
   } else {
     paramList.emplace_back("::llvm::SmallVectorImpl<::mlir::OpFoldResult> &",