merge code for llvm.emit_c_interface into convertFuncOpToLLVMFuncOp #92986

fengxie · 2024-05-22T03:08:28Z

In today's repo, attribute llvm.emit_c_interface of func op is handled outside of mlir::convertFuncOpToLLVMFuncOp in FuncOpConversion pattern. In some cases, FuncOpConversion can't be directly re-used, but we still want to re-use the code to emit c interface for llvm.emit_c_interface.

Changes in this PR

move the code to generate c with "llvm.emit_c_interface" interface into mlir::convertFuncOpToLLVMFuncOp to be able to re-use it.
added unit test to verify c interface for jit can be generated correctly if only call convertFuncOpToLLVMFuncOp.
removed FuncOpConversionBase

llvmbot · 2024-05-22T03:08:57Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: drazi (fengxie)

Changes

In today's repo, attribute llvm.emit_c_interface of func op is handled outside of mlir::convertFuncOpToLLVMFuncOp in FuncOpConversion pattern. In some cases, FuncOpConversion can't be directly re-used, but we still want to re-use the code to emit c interface for llvm.emit_c_interface.

This PR

move the code to generate c with "llvm.emit_c_interface" interface into mlir::convertFuncOpToLLVMFuncOp to be able to re-use it.
added unit test to verify c interface for jit can be generated correctly.

Full diff: https://github.com/llvm/llvm-project/pull/92986.diff

5 Files Affected:

(modified) mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp (+20-20)
(added) mlir/test/Transforms/test-convert-func-op.mlir (+12)
(modified) mlir/test/lib/Conversion/FuncToLLVM/CMakeLists.txt (+1)
(added) mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp (+62)
(modified) mlir/tools/mlir-opt/mlir-opt.cpp (+2)

diff --git a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
index 53b44aa3241bb..202bbd6fd17ab 100644
--- a/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
+++ b/mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
@@ -449,6 +449,26 @@ mlir::convertFuncOpToLLVMFuncOp(FunctionOpInterface funcOp,
                                        "region types conversion failed");
   }
 
+  if (!shouldUseBarePtrCallConv(funcOp, &converter)) {
+    if (funcOp->getAttrOfType<UnitAttr>(
+            LLVM::LLVMDialect::getEmitCWrapperAttrName())) {
+      if (newFuncOp.isVarArg())
+        return funcOp.emitError("C interface for variadic functions is not "
+                                "supported yet.");
+
+      if (newFuncOp.isExternal())
+        wrapExternalFunction(rewriter, funcOp->getLoc(), converter, funcOp,
+                             newFuncOp);
+      else
+        wrapForExternalCallers(rewriter, funcOp->getLoc(), converter, funcOp,
+                               newFuncOp);
+    }
+  } else {
+    modifyFuncOpToUseBarePtrCallingConv(
+        rewriter, funcOp->getLoc(), converter, newFuncOp,
+        llvm::cast<FunctionType>(funcOp.getFunctionType()).getInputs());
+  }
+
   return newFuncOp;
 }
 
@@ -484,26 +504,6 @@ struct FuncOpConversion : public FuncOpConversionBase {
     if (failed(newFuncOp))
       return rewriter.notifyMatchFailure(funcOp, "Could not convert funcop");
 
-    if (!shouldUseBarePtrCallConv(funcOp, this->getTypeConverter())) {
-      if (funcOp->getAttrOfType<UnitAttr>(
-              LLVM::LLVMDialect::getEmitCWrapperAttrName())) {
-        if (newFuncOp->isVarArg())
-          return funcOp->emitError("C interface for variadic functions is not "
-                                   "supported yet.");
-
-        if (newFuncOp->isExternal())
-          wrapExternalFunction(rewriter, funcOp->getLoc(), *getTypeConverter(),
-                               funcOp, *newFuncOp);
-        else
-          wrapForExternalCallers(rewriter, funcOp->getLoc(),
-                                 *getTypeConverter(), funcOp, *newFuncOp);
-      }
-    } else {
-      modifyFuncOpToUseBarePtrCallingConv(rewriter, funcOp->getLoc(),
-                                          *getTypeConverter(), *newFuncOp,
-                                          funcOp.getFunctionType().getInputs());
-    }
-
     rewriter.eraseOp(funcOp);
     return success();
   }
diff --git a/mlir/test/Transforms/test-convert-func-op.mlir b/mlir/test/Transforms/test-convert-func-op.mlir
new file mode 100644
index 0000000000000..6e96703cda578
--- /dev/null
+++ b/mlir/test/Transforms/test-convert-func-op.mlir
@@ -0,0 +1,12 @@
+// RUN: mlir-opt %s -test-convert-func-op | FileCheck %s
+
+// CHECK-LABEL: llvm.func @add
+func.func @add(%arg0: i32, %arg1: i32) -> i32 attributes { llvm.emit_c_interface } {
+  %res = arith.addi %arg0, %arg1 : i32
+  return %res : i32
+}
+// CHECK-LABEL: llvm.func @_mlir_ciface_add
+// CHECK-SAME: [[ARG0:%[a-zA-Z0-9_]+]]: i32
+// CHECK-SAME: [[ARG1:%[a-zA-Z0-9_]+]]: i32
+// CHECK-NEXT: [[RES:%.*]] = llvm.call @add([[ARG0]], [[ARG1]])
+// CHECK-NEXT: llvm.return [[RES]]
diff --git a/mlir/test/lib/Conversion/FuncToLLVM/CMakeLists.txt b/mlir/test/lib/Conversion/FuncToLLVM/CMakeLists.txt
index 45ba62d839d36..d3dbc94a99bc7 100644
--- a/mlir/test/lib/Conversion/FuncToLLVM/CMakeLists.txt
+++ b/mlir/test/lib/Conversion/FuncToLLVM/CMakeLists.txt
@@ -1,6 +1,7 @@
 # Exclude tests from libMLIR.so
 add_mlir_library(MLIRTestFuncToLLVM
   TestConvertCallOp.cpp
+  TestConvertFuncOp.cpp
 
   EXCLUDE_FROM_LIBMLIR
 
diff --git a/mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp b/mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp
new file mode 100644
index 0000000000000..87fbf2dd15917
--- /dev/null
+++ b/mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp
@@ -0,0 +1,62 @@
+//===- TestConvertFuncOp.cpp - Test LLVM Conversion of Func FuncOp --------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestDialect.h"
+
+#include "mlir/Conversion/FuncToLLVM/ConvertFuncToLLVM.h"
+#include "mlir/Conversion/LLVMCommon/Pattern.h"
+#include "mlir/Dialect/Func/IR/FuncOps.h"
+#include "mlir/Dialect/LLVMIR/LLVMDialect.h"
+#include "mlir/Pass/Pass.h"
+
+using namespace mlir;
+
+namespace {
+
+struct TestConvertFuncOp
+    : public PassWrapper<TestConvertFuncOp, OperationPass<ModuleOp>> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestConvertFuncOp)
+
+  void getDependentDialects(DialectRegistry &registry) const final {
+    registry.insert<LLVM::LLVMDialect>();
+  }
+  [[nodiscard]] StringRef getArgument() const final {
+    return "test-convert-func-op";
+  }
+  [[nodiscard]] StringRef getDescription() const final {
+    return "Tests conversion of `func.func` to `llvm.func` for different "
+           "attributes";
+  }
+
+  void runOnOperation() override {
+    ModuleOp m = getOperation();
+
+    LowerToLLVMOptions options(m.getContext());
+
+    // Populate type conversions.
+    LLVMTypeConverter typeConverter(m.getContext(), options);
+
+    // Populate patterns.
+    RewritePatternSet patterns(m.getContext());
+    populateFuncToLLVMConversionPatterns(typeConverter, patterns);
+
+    // Set target.
+    ConversionTarget target(getContext());
+    target.addLegalDialect<LLVM::LLVMDialect>();
+    target.addIllegalDialect<func::FuncDialect>();
+
+    if (failed(applyPartialConversion(m, target, std::move(patterns))))
+      signalPassFailure();
+  }
+};
+
+} // namespace
+
+namespace mlir::test {
+void registerConvertFuncOpPass() { PassRegistration<TestConvertFuncOp>(); }
+} // namespace mlir::test
diff --git a/mlir/tools/mlir-opt/mlir-opt.cpp b/mlir/tools/mlir-opt/mlir-opt.cpp
index 1dfc5d178b614..0e8b161d51345 100644
--- a/mlir/tools/mlir-opt/mlir-opt.cpp
+++ b/mlir/tools/mlir-opt/mlir-opt.cpp
@@ -71,6 +71,7 @@ namespace test {
 void registerTestCompositePass();
 void registerCommutativityUtils();
 void registerConvertCallOpPass();
+void registerConvertFuncOpPass();
 void registerInliner();
 void registerMemRefBoundCheck();
 void registerPatternsTestPass();
@@ -199,6 +200,7 @@ void registerTestPasses() {
   mlir::test::registerTestCompositePass();
   mlir::test::registerCommutativityUtils();
   mlir::test::registerConvertCallOpPass();
+  mlir::test::registerConvertFuncOpPass();
   mlir::test::registerInliner();
   mlir::test::registerMemRefBoundCheck();
   mlir::test::registerPatternsTestPass();

joker-eph · 2024-05-22T03:12:52Z

The change LGTM but I'd want to ask @ftynse : do you see a reason why the pattern included this code and not the "utility API" before?

…FuncOpConversionBase

fengxie · 2024-05-22T05:01:00Z

Updated test to only test utility function convertFuncOpToLLVMFuncOp which is main purpose of this change.

mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp

ftynse

Thanks for the patch! I suppose this is the case of "nobody needed this yet".

Can we do one step further and actually generalize this function to be usable without conversion patterns? It can take a RewriterBase & instead of ConversionPatternRewriter as far as I can see. Then it becomes usable standalone and the test can be simplified to actually test the function and not the function + conversion infra.

ftynse · 2024-05-22T09:15:48Z

mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp

+    RewritePatternSet patterns(ctx);
+    patterns.add<FuncOpConversion>(ctx);
+
+    populateFuncToLLVMConversionPatterns(typeConverter, patterns, nullptr);


Wouldn't this add the regular func conversion pattern in addition to the one added above? So we don't actually know what we are testing?

Thanks a lot for suggestion. I actually started with RewriterBase &. But RewriterBase doesn't have inlineRegionBefore and convertRegionTypes. I prefer RewriterBase & if it works. Do you have suggestion on it?

rewriter.inlineRegionBefore(funcOp.getFunctionBody(), newFuncOp.getBody(), newFuncOp.end()); if (failed(rewriter.convertRegionTypes(&newFuncOp.getBody(), converter, &result))) { return rewriter.notifyMatchFailure(funcOp, "region types conversion failed"); }

Wouldn't this add the regular func conversion pattern in addition to the one added above? So we don't actually know what we are testing?

Yes, this adds regular func conversion patterns. I created a test only have "llvm.emit_c_interface" to only test it is handled by this function.

I don't see why we need the regular func conversion pattern here. It should be enough to exercise the new pattern, and we want tests to be as minimal as possible.

Ah, thanks for explanation. I think I misunderstood your feedback. What I need to do is to delete this line right?

populateFuncToLLVMConversionPatterns(typeConverter, patterns, nullptr);

I just deleted this line and replaced it with pattern conversion for return op to llvm ( I need this to convert full func op ). Please let me know if this addressed the comment. Thanks in advance!

Technically we don't even need to use a pattern, just a walk() should do right?

For this one, I have to use pattern as convertFuncOpToLLVMFuncOp requires pattern rewriter which I can't create outside of pattern. I started with walk + RewriteBase & in the beginning but switch back to pattern.

patterns.add<FuncOpConversion>(ctx);

For ReturnOpConversion, I choose to use pattern as I already have pattern rewriter setup in place for funcOp.

@ftynse if got a chance, can you review latest changes are good to address feedbacks?

deleting general pattern and replace it with return op -> llvm.return pattern.

populateFuncToLLVMConversionPatterns(typeConverter, patterns, nullptr);

mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp

ftynse · 2024-05-22T13:28:20Z

I actually started with RewriterBase &. But RewriterBase doesn't have inlineRegionBefore and convertRegionTypes. I prefer RewriterBase & if it works. Do you have suggestion on it?

Thanks for checking. In this case, it looks easier to just keep the ConversionPatternRewriter. The alternative would be to pass in a callback, but that's too much fuss. Please address the comments about the test and this will be good to go.

fengxie · 2024-05-22T15:00:27Z

@ftynse please let me know if all comments are addressed

[[nodiscard]] was removed
Regarding this merge code for llvm.emit_c_interface into convertFuncOpToLLVMFuncOp #92986 (comment), does reply here address the comment? merge code for llvm.emit_c_interface into convertFuncOpToLLVMFuncOp #92986 (comment)

joker-eph

LG, but let's wait for @ftynse !

ftynse

Thanks!

fengxie added 2 commits May 22, 2024 10:59

merge code for llvm.emit_c_interface into convertFuncOpToLLVMFuncOp

2287e36

fixed comments and remove un-used headers

319ef33

llvmbot added mlir:core MLIR Core Infrastructure mlir labels May 22, 2024

joker-eph requested a review from ftynse May 22, 2024 03:12

fengxie added 2 commits May 22, 2024 12:51

updated test to only test mlir::convertFuncOpToLLVMFuncOp and remove …

0849489

…FuncOpConversionBase

delete unused include

0ee4c54

grypp reviewed May 22, 2024

View reviewed changes

mlir/test/lib/Conversion/FuncToLLVM/TestConvertFuncOp.cpp Outdated Show resolved Hide resolved

ftynse requested changes May 22, 2024

View reviewed changes

remove nodiscard

f12d1a0

fengxie requested review from ftynse and grypp May 22, 2024 15:43

joker-eph approved these changes May 22, 2024

View reviewed changes

remove populateFuncToLLVMConversionPatterns

b0d22d2

ftynse approved these changes May 23, 2024

View reviewed changes

ftynse merged commit a449021 into llvm:main May 23, 2024
4 checks passed

keith added a commit that referenced this pull request May 23, 2024

[bazel] Port #92986 (#93248)

fafc627

fengxie deleted the handle-emit-c-interface-in-func-op-to-llvm-func branch May 24, 2024 00:20

merge code for llvm.emit_c_interface into convertFuncOpToLLVMFuncOp #92986

merge code for llvm.emit_c_interface into convertFuncOpToLLVMFuncOp #92986

Uh oh!

Conversation

fengxie commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph commented May 22, 2024

Uh oh!

fengxie commented May 22, 2024

Uh oh!

Uh oh!

ftynse left a comment

Choose a reason for hiding this comment

Uh oh!

ftynse May 22, 2024

Choose a reason for hiding this comment

Uh oh!

fengxie May 22, 2024

Choose a reason for hiding this comment

Uh oh!

fengxie May 22, 2024

Choose a reason for hiding this comment

Uh oh!

ftynse May 22, 2024

Choose a reason for hiding this comment

Uh oh!

fengxie May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joker-eph May 23, 2024

Choose a reason for hiding this comment

Uh oh!

fengxie May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fengxie May 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ftynse commented May 22, 2024

Uh oh!

fengxie commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph left a comment

Choose a reason for hiding this comment

Uh oh!

ftynse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fengxie commented May 22, 2024 •

edited

Loading

llvmbot commented May 22, 2024 •

edited

Loading

fengxie May 22, 2024 •

edited

Loading

fengxie May 23, 2024 •

edited

Loading

fengxie commented May 22, 2024 •

edited

Loading