Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir] share argument attributes interface between calls and callables #123176

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

jeanPerier
Copy link
Contributor

@jeanPerier jeanPerier commented Jan 16, 2025

First patch of this RFC to add argument and result attributes on llvm.call (and llvm.invoke).

This patch shares core interface methods dealing with argument and result attributes from CallableOpInterface with the CallOpInterface and makes them mandatory to gives more consistent guarantees about concrete operations using these interfaces.

Add optional arg_attrs and res_attrs attributes to operations using these interfaces that did not have that already.
They can then re-use the common "rich function signature" printing/parsing helpers if they want (for the LLVM dialect, this is done in the next patch).

[edit: change the inheritance level after RFC feedback]
[edit 2: remove extra interface and use listconcat instead to share methods. Make them mandatory].

@llvmbot
Copy link
Member

llvmbot commented Jan 16, 2025

@llvm/pr-subscribers-flang-codegen
@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-mlir-core
@llvm/pr-subscribers-mlir-func
@llvm/pr-subscribers-mlir-shape
@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-mlir-gpu
@llvm/pr-subscribers-mlir-emitc

@llvm/pr-subscribers-mlir

Author: None (jeanPerier)

Changes

First patch of this RFC to add argument and result attributes on llvm.call (and llvm.invoke).

This patch extracts the core interface methods dealing with argument and result attributes from CallOpInterface in a new interface inherited by both CallOpInterface and CallableInterface.

This allows dialects to "opt-in" having parameter attributes on call arguments by adding arg_attrs and res_attrs OptionalAttr<DictArrayAttr> to the operation definition. They can then re-use the common "rich function signature" printing/parsing helpers if they want.


Patch is 34.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123176.diff

14 Files Affected:

  • (modified) mlir/docs/Interfaces.md (+9-7)
  • (added) mlir/include/mlir/Interfaces/CallImplementation.h (+91)
  • (modified) mlir/include/mlir/Interfaces/CallInterfaces.td (+56-43)
  • (modified) mlir/include/mlir/Interfaces/FunctionImplementation.h (+8-16)
  • (modified) mlir/lib/Dialect/Async/IR/Async.cpp (+1-1)
  • (modified) mlir/lib/Dialect/EmitC/IR/EmitC.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Func/IR/FuncOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/GPU/IR/GPUDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp (+2-2)
  • (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Shape/IR/Shape.cpp (+1-1)
  • (modified) mlir/lib/Interfaces/CMakeLists.txt (+5-16)
  • (added) mlir/lib/Interfaces/CallImplementation.cpp (+178)
  • (modified) mlir/lib/Interfaces/FunctionImplementation.cpp (+4-144)
diff --git a/mlir/docs/Interfaces.md b/mlir/docs/Interfaces.md
index 51747db546bb76..195f58dfbc23b3 100644
--- a/mlir/docs/Interfaces.md
+++ b/mlir/docs/Interfaces.md
@@ -753,7 +753,15 @@ interface section goes as follows:
     -   (`C++ class` -- `ODS class`(if applicable))
 
 ##### CallInterfaces
-
+*   `OpWithArgumentAttributesInterface`  - Used to represent operations that may
+    carry argument and result attributes. It is inherited by both
+    CallOpInterface and CallableOpInterface.
+    -   `ArrayAttr getArgAttrsAttr()`
+    -   `ArrayAttr getResAttrsAttr()`
+    -   `void setArgAttrsAttr(ArrayAttr)`
+    -   `void setResAttrsAttr(ArrayAttr)`
+    -   `Attribute removeArgAttrsAttr()`
+    -   `Attribute removeResAttrsAttr()`
 *   `CallOpInterface` - Used to represent operations like 'call'
     -   `CallInterfaceCallable getCallableForCallee()`
     -   `void setCalleeFromCallable(CallInterfaceCallable)`
@@ -761,12 +769,6 @@ interface section goes as follows:
     -   `Region * getCallableRegion()`
     -   `ArrayRef<Type> getArgumentTypes()`
     -   `ArrayRef<Type> getResultsTypes()`
-    -   `ArrayAttr getArgAttrsAttr()`
-    -   `ArrayAttr getResAttrsAttr()`
-    -   `void setArgAttrsAttr(ArrayAttr)`
-    -   `void setResAttrsAttr(ArrayAttr)`
-    -   `Attribute removeArgAttrsAttr()`
-    -   `Attribute removeResAttrsAttr()`
 
 ##### RegionKindInterfaces
 
diff --git a/mlir/include/mlir/Interfaces/CallImplementation.h b/mlir/include/mlir/Interfaces/CallImplementation.h
new file mode 100644
index 00000000000000..85e47f6b3dbbb9
--- /dev/null
+++ b/mlir/include/mlir/Interfaces/CallImplementation.h
@@ -0,0 +1,91 @@
+//===- CallImplementation.h - Call and Callable Op utilities ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides utility functions for implementing call-like and
+// callable-like operations, in particular, parsing, printing and verification
+// components common to these operations.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_INTERFACES_CALLIMPLEMENTATION_H
+#define MLIR_INTERFACES_CALLIMPLEMENTATION_H
+
+#include "mlir/IR/OpImplementation.h"
+#include "mlir/Interfaces/CallInterfaces.h"
+
+namespace mlir {
+
+class OpWithArgumentAttributesInterface;
+
+namespace call_interface_impl {
+
+/// Parse a function or call result list.
+///
+///   function-result-list ::= function-result-list-parens
+///                          | non-function-type
+///   function-result-list-parens ::= `(` `)`
+///                                 | `(` function-result-list-no-parens `)`
+///   function-result-list-no-parens ::= function-result (`,` function-result)*
+///   function-result ::= type attribute-dict?
+///
+ParseResult
+parseFunctionResultList(OpAsmParser &parser, SmallVectorImpl<Type> &resultTypes,
+                        SmallVectorImpl<DictionaryAttr> &resultAttrs);
+
+/// Parses a function signature using `parser`. This does not deal with function
+/// signatures containing SSA region arguments (to parse these signatures, use
+/// function_interface_impl::parseFunctionSignature). When
+/// `mustParseEmptyResult`, `-> ()` is expected when there is no result type.
+///
+///   no-ssa-function-signature ::= `(` no-ssa-function-arg-list `)`
+///                               -> function-result-list
+///   no-ssa-function-arg-list  ::= no-ssa-function-arg
+///                               (`,` no-ssa-function-arg)*
+///   no-ssa-function-arg       ::= type attribute-dict?
+ParseResult parseFunctionSignature(OpAsmParser &parser,
+                                   SmallVectorImpl<Type> &argTypes,
+                                   SmallVectorImpl<DictionaryAttr> &argAttrs,
+                                   SmallVectorImpl<Type> &resultTypes,
+                                   SmallVectorImpl<DictionaryAttr> &resultAttrs,
+                                   bool mustParseEmptyResult = true);
+
+/// Print a function signature for a call or callable operation. If a body
+/// region is provided, the SSA arguments are printed in the signature. When
+/// `printEmptyResult` is false, `-> function-result-list` is omitted when
+/// `resultTypes` is empty.
+///
+///   function-signature     ::= ssa-function-signature
+///                            | no-ssa-function-signature
+///   ssa-function-signature ::= `(` ssa-function-arg-list `)`
+///                            -> function-result-list
+///   ssa-function-arg-list  ::= ssa-function-arg (`,` ssa-function-arg)*
+///   ssa-function-arg       ::= `%`name `:` type attribute-dict?
+void printFunctionSignature(OpAsmPrinter &p,
+                            OpWithArgumentAttributesInterface op,
+                            TypeRange argTypes, bool isVariadic,
+                            TypeRange resultTypes, Region *body = nullptr,
+                            bool printEmptyResult = true);
+
+/// Adds argument and result attributes, provided as `argAttrs` and
+/// `resultAttrs` arguments, to the list of operation attributes in `result`.
+/// Internally, argument and result attributes are stored as dict attributes
+/// with special names given by getResultAttrName, getArgumentAttrName.
+void addArgAndResultAttrs(Builder &builder, OperationState &result,
+                          ArrayRef<DictionaryAttr> argAttrs,
+                          ArrayRef<DictionaryAttr> resultAttrs,
+                          StringAttr argAttrsName, StringAttr resAttrsName);
+void addArgAndResultAttrs(Builder &builder, OperationState &result,
+                          ArrayRef<OpAsmParser::Argument> args,
+                          ArrayRef<DictionaryAttr> resultAttrs,
+                          StringAttr argAttrsName, StringAttr resAttrsName);
+
+} // namespace call_interface_impl
+
+} // namespace mlir
+
+#endif // MLIR_INTERFACES_CALLIMPLEMENTATION_H
diff --git a/mlir/include/mlir/Interfaces/CallInterfaces.td b/mlir/include/mlir/Interfaces/CallInterfaces.td
index c6002da0d491ce..80912a9762187e 100644
--- a/mlir/include/mlir/Interfaces/CallInterfaces.td
+++ b/mlir/include/mlir/Interfaces/CallInterfaces.td
@@ -17,12 +17,65 @@
 
 include "mlir/IR/OpBase.td"
 
+
+/// Interface for operations with arguments attributes (both call-like
+/// and callable operations).
+def OpWithArgumentAttributesInterface : OpInterface<"OpWithArgumentAttributesInterface"> {
+  let description = [{
+    A call-like or callable operation that may define attributes for its arguments. 
+  }];
+  let cppNamespace = "::mlir";
+  let methods = [
+    InterfaceMethod<[{
+        Get the array of argument attribute dictionaries. The method should
+        return an array attribute containing only dictionary attributes equal in
+        number to the number of arguments. Alternatively, the method can
+        return null to indicate that the region has no argument attributes.
+      }],
+      "::mlir::ArrayAttr", "getArgAttrsAttr", (ins),
+      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
+    InterfaceMethod<[{
+        Get the array of result attribute dictionaries. The method should return
+        an array attribute containing only dictionary attributes equal in number
+        to the number of results. Alternatively, the method can return
+        null to indicate that the region has no result attributes.
+      }],
+      "::mlir::ArrayAttr", "getResAttrsAttr", (ins),
+      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
+    InterfaceMethod<[{
+      Set the array of argument attribute dictionaries.
+    }],
+    "void", "setArgAttrsAttr", (ins "::mlir::ArrayAttr":$attrs),
+      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return; }]>,
+    InterfaceMethod<[{
+      Set the array of result attribute dictionaries.
+    }],
+    "void", "setResAttrsAttr", (ins "::mlir::ArrayAttr":$attrs),
+      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return; }]>,
+    InterfaceMethod<[{
+      Remove the array of argument attribute dictionaries. This is the same as
+      setting all argument attributes to an empty dictionary. The method should
+      return the removed attribute.
+    }],
+    "::mlir::Attribute", "removeArgAttrsAttr", (ins),
+      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
+    InterfaceMethod<[{
+      Remove the array of result attribute dictionaries. This is the same as
+      setting all result attributes to an empty dictionary. The method should
+      return the removed attribute.
+    }],
+    "::mlir::Attribute", "removeResAttrsAttr", (ins),
+      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
+  ];
+}
+
 // `CallInterfaceCallable`: This is a type used to represent a single callable
 // region. A callable is either a symbol, or an SSA value, that is referenced by
 // a call-like operation. This represents the destination of the call.
 
 /// Interface for call-like operations.
-def CallOpInterface : OpInterface<"CallOpInterface"> {
+def CallOpInterface : OpInterface<"CallOpInterface",
+    [OpWithArgumentAttributesInterface]> {
   let description = [{
     A call-like operation is one that transfers control from one sub-routine to
     another. These operations may be traditional direct calls `call @foo`, or
@@ -85,7 +138,8 @@ def CallOpInterface : OpInterface<"CallOpInterface"> {
 }
 
 /// Interface for callable operations.
-def CallableOpInterface : OpInterface<"CallableOpInterface"> {
+def CallableOpInterface : OpInterface<"CallableOpInterface",
+    [OpWithArgumentAttributesInterface]> {
   let description = [{
     A callable operation is one who represents a potential sub-routine, and may
     be a target for a call-like operation (those providing the CallOpInterface
@@ -113,47 +167,6 @@ def CallableOpInterface : OpInterface<"CallableOpInterface"> {
       allow for this method may be called on function declarations).
     }],
     "::llvm::ArrayRef<::mlir::Type>", "getResultTypes">,
-
-    InterfaceMethod<[{
-        Get the array of argument attribute dictionaries. The method should
-        return an array attribute containing only dictionary attributes equal in
-        number to the number of region arguments. Alternatively, the method can
-        return null to indicate that the region has no argument attributes.
-      }],
-      "::mlir::ArrayAttr", "getArgAttrsAttr", (ins),
-      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
-    InterfaceMethod<[{
-        Get the array of result attribute dictionaries. The method should return
-        an array attribute containing only dictionary attributes equal in number
-        to the number of region results. Alternatively, the method can return
-        null to indicate that the region has no result attributes.
-      }],
-      "::mlir::ArrayAttr", "getResAttrsAttr", (ins),
-      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
-    InterfaceMethod<[{
-      Set the array of argument attribute dictionaries.
-    }],
-    "void", "setArgAttrsAttr", (ins "::mlir::ArrayAttr":$attrs),
-      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return; }]>,
-    InterfaceMethod<[{
-      Set the array of result attribute dictionaries.
-    }],
-    "void", "setResAttrsAttr", (ins "::mlir::ArrayAttr":$attrs),
-      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return; }]>,
-    InterfaceMethod<[{
-      Remove the array of argument attribute dictionaries. This is the same as
-      setting all argument attributes to an empty dictionary. The method should
-      return the removed attribute.
-    }],
-    "::mlir::Attribute", "removeArgAttrsAttr", (ins),
-      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
-    InterfaceMethod<[{
-      Remove the array of result attribute dictionaries. This is the same as
-      setting all result attributes to an empty dictionary. The method should
-      return the removed attribute.
-    }],
-    "::mlir::Attribute", "removeResAttrsAttr", (ins),
-      /*methodBody=*/[{}], /*defaultImplementation=*/[{ return nullptr; }]>,
   ];
 }
 
diff --git a/mlir/include/mlir/Interfaces/FunctionImplementation.h b/mlir/include/mlir/Interfaces/FunctionImplementation.h
index a5e6963e4e666f..ae20533ef4b87c 100644
--- a/mlir/include/mlir/Interfaces/FunctionImplementation.h
+++ b/mlir/include/mlir/Interfaces/FunctionImplementation.h
@@ -16,6 +16,7 @@
 #define MLIR_IR_FUNCTIONIMPLEMENTATION_H_
 
 #include "mlir/IR/OpImplementation.h"
+#include "mlir/Interfaces/CallImplementation.h"
 #include "mlir/Interfaces/FunctionInterfaces.h"
 
 namespace mlir {
@@ -33,19 +34,6 @@ class VariadicFlag {
   bool variadic;
 };
 
-/// Adds argument and result attributes, provided as `argAttrs` and
-/// `resultAttrs` arguments, to the list of operation attributes in `result`.
-/// Internally, argument and result attributes are stored as dict attributes
-/// with special names given by getResultAttrName, getArgumentAttrName.
-void addArgAndResultAttrs(Builder &builder, OperationState &result,
-                          ArrayRef<DictionaryAttr> argAttrs,
-                          ArrayRef<DictionaryAttr> resultAttrs,
-                          StringAttr argAttrsName, StringAttr resAttrsName);
-void addArgAndResultAttrs(Builder &builder, OperationState &result,
-                          ArrayRef<OpAsmParser::Argument> args,
-                          ArrayRef<DictionaryAttr> resultAttrs,
-                          StringAttr argAttrsName, StringAttr resAttrsName);
-
 /// Callback type for `parseFunctionOp`, the callback should produce the
 /// type that will be associated with a function-like operation from lists of
 /// function arguments and results, VariadicFlag indicates whether the function
@@ -84,9 +72,13 @@ void printFunctionOp(OpAsmPrinter &p, FunctionOpInterface op, bool isVariadic,
 
 /// Prints the signature of the function-like operation `op`. Assumes `op` has
 /// is a FunctionOpInterface and has passed verification.
-void printFunctionSignature(OpAsmPrinter &p, FunctionOpInterface op,
-                            ArrayRef<Type> argTypes, bool isVariadic,
-                            ArrayRef<Type> resultTypes);
+inline void printFunctionSignature(OpAsmPrinter &p, FunctionOpInterface op,
+                                   ArrayRef<Type> argTypes, bool isVariadic,
+                                   ArrayRef<Type> resultTypes) {
+  call_interface_impl::printFunctionSignature(p, op, argTypes, isVariadic,
+                                              resultTypes, &op->getRegion(0),
+                                              /*printEmptyResult=*/false);
+}
 
 /// Prints the list of function prefixed with the "attributes" keyword. The
 /// attributes with names listed in "elided" as well as those used by the
diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp
index a3e3f80954efce..d3bb250bb8ab9d 100644
--- a/mlir/lib/Dialect/Async/IR/Async.cpp
+++ b/mlir/lib/Dialect/Async/IR/Async.cpp
@@ -308,7 +308,7 @@ void FuncOp::build(OpBuilder &builder, OperationState &state, StringRef name,
   if (argAttrs.empty())
     return;
   assert(type.getNumInputs() == argAttrs.size());
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, state, argAttrs, /*resultAttrs=*/std::nullopt,
       getArgAttrsAttrName(state.name), getResAttrsAttrName(state.name));
 }
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index fdc21d6c6e24b9..6af17087ced968 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -529,7 +529,7 @@ void FuncOp::build(OpBuilder &builder, OperationState &state, StringRef name,
   if (argAttrs.empty())
     return;
   assert(type.getNumInputs() == argAttrs.size());
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, state, argAttrs, /*resultAttrs=*/std::nullopt,
       getArgAttrsAttrName(state.name), getResAttrsAttrName(state.name));
 }
diff --git a/mlir/lib/Dialect/Func/IR/FuncOps.cpp b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
index a490b4c3c4ab43..ba7b84f27d6a8a 100644
--- a/mlir/lib/Dialect/Func/IR/FuncOps.cpp
+++ b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
@@ -190,7 +190,7 @@ void FuncOp::build(OpBuilder &builder, OperationState &state, StringRef name,
   if (argAttrs.empty())
     return;
   assert(type.getNumInputs() == argAttrs.size());
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, state, argAttrs, /*resultAttrs=*/std::nullopt,
       getArgAttrsAttrName(state.name), getResAttrsAttrName(state.name));
 }
diff --git a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
index 49209229259a73..8b85c0829acfec 100644
--- a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
+++ b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
@@ -1487,7 +1487,7 @@ ParseResult GPUFuncOp::parse(OpAsmParser &parser, OperationState &result) {
   result.addAttribute(getFunctionTypeAttrName(result.name),
                       TypeAttr::get(type));
 
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, result, entryArgs, resultAttrs, getArgAttrsAttrName(result.name),
       getResAttrsAttrName(result.name));
 
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index ef5f1b069b40a3..ef1e0222e05f06 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -2510,7 +2510,7 @@ void LLVMFuncOp::build(OpBuilder &builder, OperationState &result,
 
   assert(llvm::cast<LLVMFunctionType>(type).getNumParams() == argAttrs.size() &&
          "expected as many argument attribute lists as arguments");
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, result, argAttrs, /*resultAttrs=*/std::nullopt,
       getArgAttrsAttrName(result.name), getResAttrsAttrName(result.name));
 }
@@ -2636,7 +2636,7 @@ ParseResult LLVMFuncOp::parse(OpAsmParser &parser, OperationState &result) {
 
   if (failed(parser.parseOptionalAttrDictWithKeyword(result.attributes)))
     return failure();
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       parser.getBuilder(), result, entryArgs, resultAttrs,
       getArgAttrsAttrName(result.name), getResAttrsAttrName(result.name));
 
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
index 26559c1321db5e..870359ce55301c 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
@@ -940,7 +940,7 @@ ParseResult spirv::FuncOp::parse(OpAsmParser &parser, OperationState &result) {
 
   // Add the attributes to the function arguments.
   assert(resultAttrs.size() == resultTypes.size());
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, result, entryArgs, resultAttrs, getArgAttrsAttrName(result.name),
       getResAttrsAttrName(result.name));
 
diff --git a/mlir/lib/Dialect/Shape/IR/Shape.cpp b/mlir/lib/Dialect/Shape/IR/Shape.cpp
index 65efc88e9c4033..2200af0f67a862 100644
--- a/mlir/lib/Dialect/Shape/IR/Shape.cpp
+++ b/mlir/lib/Dialect/Shape/IR/Shape.cpp
@@ -1297,7 +1297,7 @@ void FuncOp::build(OpBuilder &builder, OperationState &state, StringRef name,
   if (argAttrs.empty())
     return;
   assert(type.getNumInputs() == argAttrs.size());
-  function_interface_impl::addArgAndResultAttrs(
+  call_interface_impl::addArgAndResultAttrs(
       builder, state, argAttrs, /*resultAttrs=*/std::nullopt,
       getArgAttrsAttrName(state.name), getResAttrsAttrName(state.name));
 }
diff --git a/mlir/lib/Interfaces/CMakeLists.txt b/mlir/lib/Interfaces/CMakeLists.txt
index d3b7bf65ad3e73..76e2d921c2a9f5 100644
--- a/mlir/lib/Interfaces/CMakeLists.txt
+++ b/mlir/lib/Interfaces/CMakeLists.txt
@@ -1,4 +1,5 @@
 set(LLVM_OPTIONAL_SOURCES
+  CallImplementation.cpp
   CallInterfaces.cpp
   CastInterfaces.cpp
   ControlF...
[truncated]

@llvmbot llvmbot added the mlir:core MLIR Core Infrastructure label Jan 16, 2025
Copy link
Contributor

@River707 River707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general code movement seems good!

Share methods via listconcat instead.
Make methods mandatory (no default impl).
Add arg_attrs and res_attrs to all concrete operations inheriting from
CallOpInterface and CallableOpInterface.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:codegen labels Jan 21, 2025
@jeanPerier jeanPerier requested a review from clementval January 21, 2025 10:40
@jeanPerier
Copy link
Contributor Author

Thanks for the feedback @River707!

I updated this PR based on your comments and the RFC to make the methods mandatory in both CallOpInterafce and CallableOpInterface without using an extra OpInterface. This patch is now adding new optional arg_attrs and res_attrs attributes to all concrete operations inheriting these interfaces.

@jeanPerier jeanPerier requested a review from River707 January 23, 2025 09:46
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my end.

Thanks for working on this feature! I added some nits. Please pick the ones that make sense.

@jeanPerier
Copy link
Contributor Author

LGTM from my end.

Thanks for working on this feature! I added some nits. Please pick the ones that make sense.

Thanks for the review! The nits made sense to me. I applied them.

@krzysz00
Copy link
Contributor

Silly question: will this allow arg/result attributes on intrinsics?

@@ -14,6 +14,7 @@
#ifndef MLIR_INTERFACES_CALLINTERFACES_H
#define MLIR_INTERFACES_CALLINTERFACES_H

#include "mlir/IR/OpImplementation.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpImplementation.h pulls in a lot of extra junk, can you use forward declares here instead for whatever is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because of OpAsmParser::Argument usage, I think it is not possible to forward declare nested types.

Either that class should be hoisted in a new OpAsmParserArgument class (a bit outside of the scope of this patch), or the helpers need to be in a different header (maybe that is why FunctionImplementation.h exists).

What do you think, is it worth splitting the header to avoid that include?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to not split the header. Will likely merge on Monday, please tell me if you think splitting the header is better.

@jeanPerier
Copy link
Contributor Author

Silly question: will this allow arg/result attributes on intrinsics?

Not through this interface change because llvm.call_intrinsic does not use the CallOpInterface.

So if this is desired, this should be added separately on the llvm.call_intrinsic.

@jeanPerier jeanPerier merged commit 327d627 into main Feb 3, 2025
9 checks passed
@jeanPerier jeanPerier deleted the users/jeanPerier/mlir-call-attrs-iface branch February 3, 2025 10:27
jeanPerier added a commit to jeanPerier/llvm-project that referenced this pull request Feb 3, 2025
hanhanW added a commit to iree-org/llvm-project that referenced this pull request Feb 4, 2025
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
llvm#123176)

This patch shares core interface methods dealing with argument and
result attributes from CallableOpInterface with the CallOpInterface and
makes them mandatory to gives more consistent guarantees about concrete
operations using these interfaces.

This allows adding argument attributes on call like operations, which is
sometimes required to get proper ABI, like with  llvm.call (and llvm.invoke).


The patch adds optional `arg_attrs` and `res_attrs` attributes to operations using
these interfaces that did not have that already.
They can then re-use the common "rich function signature"
printing/parsing helpers if they want (for the LLVM dialect, this is
done in the next patch).

Part of RFC: https://discourse.llvm.org/t/mlir-rfc-adding-argument-and-result-attributes-to-llvm-call/84107
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants