[mlir][emitc] Add a declare_func operation
#80297
Conversation
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-emitc Author: Marius Brehler (marbre) ChangesThis adds the Full diff: https://github.com/llvm/llvm-project/pull/80297.diff 6 Files Affected:
diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
index 5c8c3c9ce7bb3..fe004e441e9f9 100644
--- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
+++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td
@@ -460,6 +460,48 @@ def EmitC_CallOp : EmitC_Op<"call",
}];
}
+def EmitC_DeclareFuncOp : EmitC_Op<"declare_func", [
+ DeclareOpInterfaceMethods<SymbolUserOpInterface>
+]> {
+ let summary = "An operation to declare a function";
+ let description = [{
+ The `declare_func` operation allows to insert a function declaration for an
+ `emitc.func` at a specific position. The operation only requires the `callee`
+ of the `emitc.func` to be specified as an attribute.
+
+ Example:
+
+ ```mlir
+ emitc.declare_func @bar
+ emitc.func @foo(%arg0: i32) -> i32 {
+ %0 = emitc.call @bar(%arg0) : (i32) -> (i32)
+ emitc.return %0 : i32
+ }
+
+ emitc.func @bar(%arg0: i32) -> i32 {
+ emitc.return %arg0 : i32
+ }
+ ```
+
+ ```c++
+ // Code emitted for the operations above.
+ int32_t bar(int32_t v1);
+ int32_t foo(int32_t v1) {
+ int32_t v2 = bar(v1);
+ return v2;
+ }
+
+ int32_t bar(int32_t v1) {
+ return v1;
+ }
+ ```
+ }];
+ let arguments = (ins FlatSymbolRefAttr:$callee);
+ let assemblyFormat = [{
+ $callee attr-dict
+ }];
+}
+
def EmitC_FuncOp : EmitC_Op<"func", [
AutomaticAllocationScope,
FunctionOpInterface, IsolatedFromAbove
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index df489c6d90fb1..67bcb7079672e 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -393,6 +393,24 @@ FunctionType CallOp::getCalleeType() {
return FunctionType::get(getContext(), getOperandTypes(), getResultTypes());
}
+//===----------------------------------------------------------------------===//
+// DeclareFuncOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult
+DeclareFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
+ // Check that the callee attribute was specified.
+ auto fnAttr = (*this)->getAttrOfType<FlatSymbolRefAttr>("callee");
+ if (!fnAttr)
+ return emitOpError("requires a 'callee' symbol reference attribute");
+ FuncOp fn = symbolTable.lookupNearestSymbolFrom<FuncOp>(*this, fnAttr);
+ if (!fn)
+ return emitOpError() << "'" << fnAttr.getValue()
+ << "' does not reference a valid function";
+
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// FuncOp
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index c0c6105409f8d..28aef02e72566 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -14,6 +14,7 @@
#include "mlir/IR/BuiltinTypes.h"
#include "mlir/IR/Dialect.h"
#include "mlir/IR/Operation.h"
+#include "mlir/IR/SymbolTable.h"
#include "mlir/Support/IndentedOstream.h"
#include "mlir/Support/LLVM.h"
#include "mlir/Target/Cpp/CppEmitter.h"
@@ -847,8 +848,9 @@ static LogicalResult printFunctionBody(CppEmitter &emitter,
// needs to be printed after the closing brace.
// When generating code for an emitc.for and emitc.verbatim op, printing a
// trailing semicolon is handled within the printOperation function.
- bool trailingSemicolon = !isa<cf::CondBranchOp, emitc::ForOp, emitc::IfOp,
- emitc::LiteralOp, emitc::VerbatimOp>(op);
+ bool trailingSemicolon =
+ !isa<cf::CondBranchOp, emitc::DeclareFuncOp, emitc::ForOp,
+ emitc::IfOp, emitc::LiteralOp, emitc::VerbatimOp>(op);
if (failed(emitter.emitOperation(
op, /*trailingSemicolon=*/trailingSemicolon)))
@@ -923,6 +925,37 @@ static LogicalResult printOperation(CppEmitter &emitter,
return success();
}
+static LogicalResult printOperation(CppEmitter &emitter,
+ DeclareFuncOp declareFuncOp) {
+ CppEmitter::Scope scope(emitter);
+ raw_indented_ostream &os = emitter.ostream();
+
+ auto functionOp = SymbolTable::lookupNearestSymbolFrom<emitc::FuncOp>(
+ declareFuncOp, declareFuncOp.getCalleeAttr());
+
+ if (!functionOp)
+ return failure();
+
+ if (functionOp.getSpecifiers()) {
+ for (Attribute specifier : functionOp.getSpecifiersAttr()) {
+ os << cast<StringAttr>(specifier).str() << " ";
+ }
+ }
+
+ if (failed(emitter.emitTypes(functionOp.getLoc(),
+ functionOp.getFunctionType().getResults())))
+ return failure();
+ os << " " << functionOp.getName();
+
+ os << "(";
+ Operation *operation = functionOp.getOperation();
+ if (failed(printFunctionArgs(emitter, operation, functionOp.getArguments())))
+ return failure();
+ os << ");";
+
+ return success();
+}
+
CppEmitter::CppEmitter(raw_ostream &os, bool declareVariablesAtTop)
: os(os), declareVariablesAtTop(declareVariablesAtTop) {
valueInScopeCount.push(0);
@@ -1236,10 +1269,10 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
// EmitC ops.
.Case<emitc::AddOp, emitc::ApplyOp, emitc::AssignOp, emitc::CallOp,
emitc::CallOpaqueOp, emitc::CastOp, emitc::CmpOp,
- emitc::ConstantOp, emitc::DivOp, emitc::ExpressionOp,
- emitc::ForOp, emitc::FuncOp, emitc::IfOp, emitc::IncludeOp,
- emitc::MulOp, emitc::RemOp, emitc::ReturnOp, emitc::SubOp,
- emitc::VariableOp, emitc::VerbatimOp>(
+ emitc::ConstantOp, emitc::DeclareFuncOp, emitc::DivOp,
+ emitc::ExpressionOp, emitc::ForOp, emitc::FuncOp, emitc::IfOp,
+ emitc::IncludeOp, emitc::MulOp, emitc::RemOp, emitc::ReturnOp,
+ emitc::SubOp, emitc::VariableOp, emitc::VerbatimOp>(
[&](auto op) { return printOperation(*this, op); })
// Func ops.
.Case<func::CallOp, func::ConstantOp, func::FuncOp, func::ReturnOp>(
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index 707f9a5b23b0b..b95fafa379ff7 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -326,3 +326,13 @@ emitc.func @func_variadic(...)
// expected-error@+1 {{'emitc.func' op does not support empty function bodies}}
emitc.func private @empty()
+
+// -----
+
+// expected-error@+1 {{'emitc.declare_func' op 'bar' does not reference a valid function}}
+emitc.declare_func @bar
+
+// -----
+
+// expected-error@+1 {{'emitc.declare_func' op requires attribute 'callee'}}
+"emitc.declare_func"() : () -> ()
diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir
index b41333faa4d4e..81ad5a2b46334 100644
--- a/mlir/test/Dialect/EmitC/ops.mlir
+++ b/mlir/test/Dialect/EmitC/ops.mlir
@@ -15,6 +15,8 @@ func.func @f(%arg0: i32, %f: !emitc.opaque<"int32_t">) {
return
}
+emitc.declare_func @func
+
emitc.func @func(%arg0 : i32) {
emitc.call_opaque "foo"(%arg0) : (i32) -> ()
emitc.return
diff --git a/mlir/test/Target/Cpp/declare_func.mlir b/mlir/test/Target/Cpp/declare_func.mlir
new file mode 100644
index 0000000000000..72c087a3388e2
--- /dev/null
+++ b/mlir/test/Target/Cpp/declare_func.mlir
@@ -0,0 +1,16 @@
+// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s
+
+// CHECK: int32_t bar(int32_t [[V1:[^ ]*]]);
+emitc.declare_func @bar
+// CHECK: int32_t bar(int32_t [[V1:[^ ]*]]) {
+emitc.func @bar(%arg0: i32) -> i32 {
+ emitc.return %arg0 : i32
+}
+
+
+// CHECK: static inline int32_t foo(int32_t [[V1:[^ ]*]]);
+emitc.declare_func @foo
+// CHECK: static inline int32_t foo(int32_t [[V1:[^ ]*]]) {
+emitc.func @foo(%arg0: i32) -> i32 attributes {specifiers = ["static","inline"]} {
+ emitc.return %arg0 : i32
+}
|
|
@AlexDenisov if you don't mind I would like to ask you for a code review. |
There was a problem hiding this comment.
That's a great and much needed feature. I used some terrible workarounds to achieve the same, and ended up using VerbatimOp.
I'd add a test case for when the prototype is emitted inside the function body as well.
It looks like this feature would be incomplete without support of external functions (mlir::func::FuncOp which are only declaring the functions).
I'd expect this case to naturally map to this operation? Otherwise the use case is rather limited.
Well @simon-camp and me already started discussion the value of a function declaration within a function body. While this is legal in C, it is not as soon as it is a static function. Therefore, I see three option:
To make sure I understand what you're proposing: A |
These two sound better than disallowing them completely.
Yes, exactly. Although, I'm not sure about the |
|
I agree with the need to declare external functions. We can't rewrite an external func.func to a emitc.declare_func though. The former provides a symbol while the latter refrences an existing symbol. We can convert it to an emitc.func with empty body and adjust the emitter to print it correctly as a declaration. Then So the emitc.declare_func is an operation that can be automatically inserted by a legalization pass to make the compiler happy for cases like mutually recursive functions. int isEven(unsigned int n) {
return n == 0 ? 1 : isOdd(n-1);
}
int isOdd(unsigned int n) {
return n == 1 = 1 : isEven(n-1);
} |
I'll prototype something and see if we have other options. If those don't work out and if we follow this approach, we could (more or less) land the current implementation of |
|
None other than resolving the two open inline discussions. 👍 |
marbre
force-pushed
the
emitc.declare_func
branch
from
213f721 to
589be75
Compare
For now I added a verifier that checks if the parent is a |
mlir/lib/Dialect/EmitC/IR/EmitC.cpp
Outdated
| auto funcOp = | ||
| SymbolTable::lookupNearestSymbolFrom<emitc::FuncOp>((*this), fnAttr); | ||
|
|
||
| if (!isa<ModuleOp>((*this)->getParentOp()) && funcOp.isPrivate()) |
There was a problem hiding this comment.
We shouldn't be using isPrivate here. The symbol visibility is not neccessarily in sync with the specifiers.
There was a problem hiding this comment.
If it's not, something other is broken but I agree that can be the case. That's another point why a validation pass might be more favorable here. Beside that, going through all specifiers args in the array adds more a much larger overhead to the verifier.
I would leave this out for now and see how it works out. Seems easier to add restrictions later. |
This adds the `emitc.declare_func` operation that allows to emit the declaration of an `emitc.func` at a specific location.
marbre
force-pushed
the
emitc.declare_func
branch
from
589be75 to
0864be4
Compare
I dropped the change and will add it to our todo list for the validation pass. |
This adds the `emitc.declare_func` operation that allows to emit the declaration of an `emitc.func` at a specific location.
This adds the
emitc.declare_funcoperation that allows to emit the declaration of anemitc.funcat a specific location.