[mlir][EmitC] Add MathToEmitC pass for math function lowering to EmitC #113799
Conversation
This commit introduces a new `MathToEmitC` conversion pass that lowers selected math operations to the `emitc.call_opaque` operation in the EmitC dialect. The supported math operations include: - math.floor -> emitc.call_opaque<"floor"> - math.exp -> emitc.call_opaque<"exp"> - math.cos -> emitc.call_opaque<"cos"> - math.sin -> emitc.call_opaque<"sin"> - math.ipowi -> emitc.call_opaque<"pow"> We chose to use `emitc.call_opaque` instead of `emitc.call` to better align with C-style function overloading. Unlike `emitc.call`, which requires unique type signatures, `emitc.call_opaque` allows us to call functions without specifying a unique type-based signature. This flexibility is essential for mimicking function overloading behavior as seen in `<math.h>`. Additionally, the pass inserts an `emitc.include` operation to generate `#include <math.h>` at the top of the module to ensure the availability of the necessary math functions in the generated code. This pass enables the use of EmitC as an intermediate layer to generate C/C++ code with opaque calls to standard math functions.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
mlir
bazel
|
@llvm/pr-subscribers-mlir-emitc @llvm/pr-subscribers-mlir Author: None (recursion-man) ChangesThis commit introduces a new The supported math operations include:
We chose to use Additionally, the pass inserts an This pass enables the use of EmitC as an intermediate layer to generate C/C++ code with opaque calls to standard math functions. Full diff: https://github.com/llvm/llvm-project/pull/113799.diff 8 Files Affected:
diff --git a/mlir/include/mlir/Conversion/MathToEmitC/MathToEmitC.h b/mlir/include/mlir/Conversion/MathToEmitC/MathToEmitC.h
new file mode 100644
index 00000000000000..f2e8779b057937
--- /dev/null
+++ b/mlir/include/mlir/Conversion/MathToEmitC/MathToEmitC.h
@@ -0,0 +1,25 @@
+//===- MathToEmitC.h - Math to EmitC Pass -----------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_CONVERSION_MATHTOEMITC_MATHTOEMITC_H
+#define MLIR_CONVERSION_MATHTOEMITC_MATHTOEMITC_H
+
+#include "mlir/IR/BuiltinOps.h"
+#include "mlir/Pass/Pass.h"
+#include <memory>
+
+namespace mlir {
+
+#define GEN_PASS_DECL_CONVERTMATHTOEMITC
+#include "mlir/Conversion/Passes.h.inc"
+
+std::unique_ptr<OperationPass<mlir::ModuleOp>> createConvertMathToEmitCPass();
+
+} // namespace mlir
+
+#endif // MLIR_CONVERSION_MATHTOEMITC_MATHTOEMITC_H
diff --git a/mlir/include/mlir/Conversion/Passes.h b/mlir/include/mlir/Conversion/Passes.h
index 2ab32836c80b1c..54e795cd137d31 100644
--- a/mlir/include/mlir/Conversion/Passes.h
+++ b/mlir/include/mlir/Conversion/Passes.h
@@ -43,6 +43,7 @@
#include "mlir/Conversion/IndexToLLVM/IndexToLLVM.h"
#include "mlir/Conversion/IndexToSPIRV/IndexToSPIRV.h"
#include "mlir/Conversion/LinalgToStandard/LinalgToStandard.h"
+#include "mlir/Conversion/MathToEmitC/MathToEmitC.h"
#include "mlir/Conversion/MathToFuncs/MathToFuncs.h"
#include "mlir/Conversion/MathToLLVM/MathToLLVM.h"
#include "mlir/Conversion/MathToLibm/MathToLibm.h"
diff --git a/mlir/include/mlir/Conversion/Passes.td b/mlir/include/mlir/Conversion/Passes.td
index 4d272ba219c6f1..09a93439ab898f 100644
--- a/mlir/include/mlir/Conversion/Passes.td
+++ b/mlir/include/mlir/Conversion/Passes.td
@@ -780,6 +780,25 @@ def ConvertMathToSPIRV : Pass<"convert-math-to-spirv"> {
let dependentDialects = ["spirv::SPIRVDialect"];
}
+//===----------------------------------------------------------------------===//
+// MathToEmitC
+//===----------------------------------------------------------------------===//
+
+def ConvertMathToEmitC : Pass<"convert-math-to-emitc", "ModuleOp"> {
+ let summary = "Convert some Math operations to EmitC Call_opaque";
+ let description = [{
+ This pass converts supported Math ops to call_opaque calls to compiler generated
+ functions implementing these operations in software.
+ Unlike convert-math-to-funcs pass, this pass uses call_opaque,
+ therefore enables us to overload the same funtion with different argument types
+ }];
+
+ let constructor = "mlir::createConvertMathToEmitCPass()";
+ let dependentDialects = ["emitc::EmitCDialect",
+ "math::MathDialect"
+ ];
+}
+
//===----------------------------------------------------------------------===//
// MathToFuncs
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Conversion/CMakeLists.txt b/mlir/lib/Conversion/CMakeLists.txt
index 6651d87162257f..120b4972454d57 100644
--- a/mlir/lib/Conversion/CMakeLists.txt
+++ b/mlir/lib/Conversion/CMakeLists.txt
@@ -33,6 +33,7 @@ add_subdirectory(IndexToLLVM)
add_subdirectory(IndexToSPIRV)
add_subdirectory(LinalgToStandard)
add_subdirectory(LLVMCommon)
+add_subdirectory(MathToEmitC)
add_subdirectory(MathToFuncs)
add_subdirectory(MathToLibm)
add_subdirectory(MathToLLVM)
diff --git a/mlir/lib/Conversion/MathToEmitC/CMakeLists.txt b/mlir/lib/Conversion/MathToEmitC/CMakeLists.txt
new file mode 100644
index 00000000000000..7b02a57dff3d4d
--- /dev/null
+++ b/mlir/lib/Conversion/MathToEmitC/CMakeLists.txt
@@ -0,0 +1,19 @@
+add_mlir_conversion_library(MLIRMathToEmitC
+ MathToEmitC.cpp
+
+ ADDITIONAL_HEADER_DIRS
+ ${MLIR_MAIN_INCLUDE_DIR}/mlir/Conversion/MathToEmitC
+
+ DEPENDS
+ MLIRConversionPassIncGen
+
+ LINK_COMPONENTS
+ Core
+
+ LINK_LIBS PUBLIC
+ MLIRLLVMCommonConversion
+ MLIREmitCDialect
+ MLIRMathDialect
+ MLIRPass
+ MLIRTransforms
+)
diff --git a/mlir/lib/Conversion/MathToEmitC/MathToEmitC.cpp b/mlir/lib/Conversion/MathToEmitC/MathToEmitC.cpp
new file mode 100644
index 00000000000000..43641a8ad634a7
--- /dev/null
+++ b/mlir/lib/Conversion/MathToEmitC/MathToEmitC.cpp
@@ -0,0 +1,99 @@
+
+//===- MathToEmitC.cpp - Math to EmitC Pass Implementation ----------===//
+//
+// 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 "mlir/Conversion/MathToEmitC/MathToEmitC.h"
+#include "mlir/Dialect/EmitC/IR/EmitC.h"
+#include "mlir/Dialect/Math/IR/Math.h"
+#include "mlir/Pass/Pass.h"
+#include "mlir/Transforms/DialectConversion.h"
+
+namespace mlir {
+#define GEN_PASS_DEF_CONVERTMATHTOEMITC
+#include "mlir/Conversion/Passes.h.inc"
+} // namespace mlir
+
+using namespace mlir;
+namespace {
+
+// Replaces Math operations with `emitc.call_opaque` operations.
+struct ConvertMathToEmitCPass
+ : public impl::ConvertMathToEmitCBase<ConvertMathToEmitCPass> {
+public:
+ void runOnOperation() final;
+};
+
+} // end anonymous namespace
+
+template <typename OpType>
+class LowerToEmitCCallOpaque : public mlir::OpRewritePattern<OpType> {
+ std::string calleeStr;
+
+public:
+ LowerToEmitCCallOpaque(MLIRContext *context, std::string calleeStr)
+ : OpRewritePattern<OpType>(context), calleeStr(calleeStr) {}
+
+ LogicalResult matchAndRewrite(OpType op,
+ PatternRewriter &rewriter) const override;
+};
+
+// Populates patterns to replace `math` operations with `emitc.call_opaque`,
+// using function names consistent with those in <math.h>.
+static void populateConvertMathToEmitCPatterns(RewritePatternSet &patterns) {
+ auto *context = patterns.getContext();
+ patterns.insert<LowerToEmitCCallOpaque<math::FloorOp>>(context, "floor");
+ patterns.insert<LowerToEmitCCallOpaque<math::RoundEvenOp>>(context, "rint");
+ patterns.insert<LowerToEmitCCallOpaque<math::ExpOp>>(context, "exp");
+ patterns.insert<LowerToEmitCCallOpaque<math::CosOp>>(context, "cos");
+ patterns.insert<LowerToEmitCCallOpaque<math::SinOp>>(context, "sin");
+ patterns.insert<LowerToEmitCCallOpaque<math::AcosOp>>(context, "acos");
+ patterns.insert<LowerToEmitCCallOpaque<math::AsinOp>>(context, "asin");
+ patterns.insert<LowerToEmitCCallOpaque<math::Atan2Op>>(context, "atan2");
+ patterns.insert<LowerToEmitCCallOpaque<math::CeilOp>>(context, "ceil");
+ patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, "fabs");
+ patterns.insert<LowerToEmitCCallOpaque<math::FPowIOp>>(context, "powf");
+ patterns.insert<LowerToEmitCCallOpaque<math::IPowIOp>>(context, "pow");
+}
+
+template <typename OpType>
+LogicalResult LowerToEmitCCallOpaque<OpType>::matchAndRewrite(
+ OpType op, PatternRewriter &rewriter) const {
+ mlir::StringAttr callee = rewriter.getStringAttr(calleeStr);
+ auto actualOp = mlir::cast<OpType>(op);
+ rewriter.replaceOpWithNewOp<mlir::emitc::CallOpaqueOp>(
+ actualOp, actualOp.getType(), callee, actualOp->getOperands());
+ return mlir::success();
+}
+
+void ConvertMathToEmitCPass::runOnOperation() {
+ auto moduleOp = getOperation();
+ // Insert #include <math.h> at the beginning of the module
+ OpBuilder builder(moduleOp.getBodyRegion());
+ builder.setInsertionPointToStart(&moduleOp.getBodyRegion().front());
+ builder.create<emitc::IncludeOp>(moduleOp.getLoc(),
+ builder.getStringAttr("math.h"));
+
+ ConversionTarget target(getContext());
+ target.addLegalOp<emitc::CallOpaqueOp>();
+
+ target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundEvenOp,
+ math::CosOp, math::SinOp, math::Atan2Op, math::CeilOp,
+ math::AcosOp, math::AsinOp, math::AbsFOp, math::PowFOp,
+ math::FPowIOp, math::IPowIOp>();
+
+ RewritePatternSet patterns(&getContext());
+ populateConvertMathToEmitCPatterns(patterns);
+
+ if (failed(applyPartialConversion(moduleOp, target, std::move(patterns))))
+ signalPassFailure();
+}
+
+std::unique_ptr<OperationPass<mlir::ModuleOp>>
+mlir::createConvertMathToEmitCPass() {
+ return std::make_unique<ConvertMathToEmitCPass>();
+}
diff --git a/mlir/test/Conversion/MathToEmitC/math-to-emitc.mlir b/mlir/test/Conversion/MathToEmitC/math-to-emitc.mlir
new file mode 100644
index 00000000000000..9add25d71ef478
--- /dev/null
+++ b/mlir/test/Conversion/MathToEmitC/math-to-emitc.mlir
@@ -0,0 +1,140 @@
+// RUN: mlir-opt --split-input-file -convert-math-to-emitc %s | FileCheck %s
+
+// CHECK-LABEL: emitc.include "math.h"
+
+// CHECK-LABEL: func.func @absf_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "fabs"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @absf_to_call_opaque(%arg0: f32) {
+ %1 = math.absf %arg0 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @floor_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "floor"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @floor_to_call_opaque(%arg0: f32) {
+ %1 = math.floor %arg0 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @sin_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "sin"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @sin_to_call_opaque(%arg0: f32) {
+ %1 = math.sin %arg0 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @cos_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "cos"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @cos_to_call_opaque(%arg0: f32) {
+ %1 = math.cos %arg0 : f32
+ return
+ }
+
+
+// -----
+
+// CHECK-LABEL: func.func @asin_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "asin"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @asin_to_call_opaque(%arg0: f32) {
+ %1 = math.asin %arg0 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @acos_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "acos"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @acos_to_call_opaque(%arg0: f32) {
+ %1 = math.acos %arg0 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @atan2_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32,
+// CHECK-SAME: %[[VAL_1:.*]]: f32) {
+// CHECK: %[[VAL_2:.*]] = emitc.call_opaque "atan2"(%[[VAL_0]], %[[VAL_1]]) : (f32, f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @atan2_to_call_opaque(%arg0: f32, %arg1: f32) {
+ %1 = math.atan2 %arg0, %arg1 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @ceil_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "ceil"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @ceil_to_call_opaque(%arg0: f32) {
+ %1 = math.ceil %arg0 : f32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @exp_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32) {
+// CHECK: %[[VAL_1:.*]] = emitc.call_opaque "exp"(%[[VAL_0]]) : (f32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @exp_to_call_opaque(%arg0: f32) {
+ %1 = math.exp %arg0 : f32
+ return
+ }
+
+
+// -----
+
+// CHECK-LABEL: func.func @fpowi_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: f32,
+// CHECK-SAME: %[[VAL_1:.*]]: i32) {
+// CHECK: %[[VAL_2:.*]] = emitc.call_opaque "powf"(%[[VAL_0]], %[[VAL_1]]) : (f32, i32) -> f32
+// CHECK: return
+// CHECK: }
+func.func @fpowi_to_call_opaque(%arg0: f32, %arg1: i32) {
+ %1 = math.fpowi %arg0, %arg1 : f32, i32
+ return
+ }
+
+// -----
+
+// CHECK-LABEL: func.func @ipowi_to_call_opaque(
+// CHECK-SAME: %[[VAL_0:.*]]: i32,
+// CHECK-SAME: %[[VAL_1:.*]]: i32) {
+// CHECK: %[[VAL_2:.*]] = emitc.call_opaque "pow"(%[[VAL_0]], %[[VAL_1]]) : (i32, i32) -> i32
+// CHECK: return
+// CHECK: }
+func.func @ipowi_to_call_opaque(%arg0: i32, %arg1: i32) {
+ %1 = math.ipowi %arg0, %arg1 : i32
+ return
+ }
+
+
diff --git a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
index 779609340d7224..b193e4295e4759 100644
--- a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
@@ -4201,6 +4201,7 @@ cc_library(
":IndexToLLVM",
":IndexToSPIRV",
":LinalgToStandard",
+ ":MathToEmitC",
":MathToFuncs",
":MathToLLVM",
":MathToLibm",
@@ -8721,6 +8722,27 @@ cc_library(
],
)
+cc_library(
+ name = "MathToEmitC",
+ srcs = glob([
+ "lib/Conversion/MathToEmitC/*.cpp",
+ ]),
+ hdrs = glob([
+ "include/mlir/Conversion/MathToEmitC/*.h",
+ ]),
+ includes = [
+ "include",
+ "lib/Conversion/MathToEmitC",
+ ],
+ deps = [
+ ":ConversionPassIncGen",
+ ":EmitCDialect",
+ ":MathDialect",
+ ":Pass",
+ ":TransformUtils",
+ ],
+)
+
cc_library(
name = "MathToFuncs",
srcs = glob(["lib/Conversion/MathToFuncs/*.cpp"]),
|
|
@marbre @TinaAMD @aniragil @simon-camp @cferry-AMD I would greatly appreciate your feedback on this PR whenever you have time. As I don't have write permissions for the repository, I am unable to assign reviewers directly. |
CoTinker
requested review from
marbre,
simon-camp,
aniragil,
TinaAMD and
cferry-AMD
| patterns.insert<LowerToEmitCCallOpaque<math::FloorOp>>(context, "floor"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::RoundEvenOp>>(context, "rint"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::ExpOp>>(context, "exp"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::CosOp>>(context, "cos"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::SinOp>>(context, "sin"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AcosOp>>(context, "acos"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AsinOp>>(context, "asin"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::Atan2Op>>(context, "atan2"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::CeilOp>>(context, "ceil"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, "fabs"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::FPowIOp>>(context, "powf"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::IPowIOp>>(context, "pow"); |
There was a problem hiding this comment.
If you have a look at the page for fabs for instance, you'll see that the C implementation (without overloading) bears a different name for each type used. How about a more generic pattern with a predicate:
patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, "fabsf", [](Type t){ return t == IntegerType::get(t.getContext(), 32); });
patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, "fabs", [](Type t){ return t == IntegerType::get(t.getContext(), 64); });
There was a problem hiding this comment.
Thanks for addressing this @cferry-AMD. This is something that we definitely should consider. However, I think it depends on what language standard one targets (see https://en.cppreference.com/w/c/numeric/math/fabs and https://en.cppreference.com/w/cpp/numeric/math/fabs). What about an optional switch to specify which standard to target as part of the conversion?
There was a problem hiding this comment.
We could also pass a pair of strings for f32 and f64 types. Or as an alternative a map<Type, string>.
There was a problem hiding this comment.
Thanks for raising this! I propose two options for handling function names:
Option 1: Hardcoded names targeting C's math.h or C++'s
C target would use type-specific suffixes (since C lacks overloading) for example :
if (operandType.isF32()) return (baseName + "f").str(); // "fabsf"
else if (operandType.isF64()) return baseName.str(); // "fabs"
C++ target would use base names with std:: for example: std::fabs
Option 2: Custom library support via user-defined mapping
std::map<TypeID, std::string> customMap = {
{TypeID::get<math::AbsFOp>(), "native_abs"},
{TypeID::get<math::ExpOp>(), "native_exp"}
};
patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, customMap);
This option allows users to pass a custom mapping, supporting other libraries (e.g., OpenCL). If no mapping is provided, it defaults to math.h.
Thanks again for your input! I’d really like to hear your opinions on these options
There was a problem hiding this comment.
How specific to Math functions do you see this pass? The second choice makes the lowering much more generic than I thought: it may apply to any op that could be lowered to opaque library calls, and not just Math dialect ops to math.h functions. I like this ability to lower anything to opaque calls with language filters, but this may not be what you intended.
There was a problem hiding this comment.
Thank you for the feedback!
I intended this pass to focus on lowering math functions specifically. I accepted the suggestion to use libraries like and am also considering the option to add custom math libraries, not just the standard ones, which motivated me to use a map. I can enforce that only math dialect ops will be converted to ensure it remains targeted.
|
Thanks for this PR @recursion-man! I second the approach to go straight to EmitC instead of through a libm detour with |
There was a problem hiding this comment.
Thanks for the PR! I think we had a discussion before that this could be nice but seems there was no strong need before.
While I like the approach, I have some concerns regarding non scalar-types. From a quick look it seems
%f = math.absf %g : tensor<4xf32>would be converted to valid EmitC and the emitter would translate to C / C++ without an error. I think this needs to be addressed as we cannot in the emitter.
| patterns.insert<LowerToEmitCCallOpaque<math::FloorOp>>(context, "floor"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::RoundEvenOp>>(context, "rint"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::ExpOp>>(context, "exp"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::CosOp>>(context, "cos"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::SinOp>>(context, "sin"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AcosOp>>(context, "acos"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AsinOp>>(context, "asin"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::Atan2Op>>(context, "atan2"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::CeilOp>>(context, "ceil"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, "fabs"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::FPowIOp>>(context, "powf"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::IPowIOp>>(context, "pow"); |
There was a problem hiding this comment.
We could also pass a pair of strings for f32 and f64 types. Or as an alternative a map<Type, string>.
This patch ensures that the MathToEmitC pass only converts scalar `FloatType`s, avoiding invalid conversions of non-scalar types like tensors. - **Validation:** Added checks to convert only scalar types. - **Refactoring:** Moved implementation to `MathToEmitCPass.cpp` and split headers. - **Testing:** Added test cases to ensure proper error handling for non-scalar types.
| patterns.insert<LowerToEmitCCallOpaque<math::FloorOp>>(context, "floor"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::RoundEvenOp>>(context, "rint"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::ExpOp>>(context, "exp"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::CosOp>>(context, "cos"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::SinOp>>(context, "sin"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AcosOp>>(context, "acos"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AsinOp>>(context, "asin"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::Atan2Op>>(context, "atan2"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::CeilOp>>(context, "ceil"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, "fabs"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::FPowIOp>>(context, "powf"); | ||
| patterns.insert<LowerToEmitCCallOpaque<math::IPowIOp>>(context, "pow"); |
There was a problem hiding this comment.
Thanks for raising this! I propose two options for handling function names:
Option 1: Hardcoded names targeting C's math.h or C++'s
C target would use type-specific suffixes (since C lacks overloading) for example :
if (operandType.isF32()) return (baseName + "f").str(); // "fabsf"
else if (operandType.isF64()) return baseName.str(); // "fabs"
C++ target would use base names with std:: for example: std::fabs
Option 2: Custom library support via user-defined mapping
std::map<TypeID, std::string> customMap = {
{TypeID::get<math::AbsFOp>(), "native_abs"},
{TypeID::get<math::ExpOp>(), "native_exp"}
};
patterns.insert<LowerToEmitCCallOpaque<math::AbsFOp>>(context, customMap);
This option allows users to pass a custom mapping, supporting other libraries (e.g., OpenCL). If no mapping is provided, it defaults to math.h.
Thanks again for your input! I’d really like to hear your opinions on these options
| target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundEvenOp, | ||
| math::CosOp, math::SinOp, math::Atan2Op, math::CeilOp, | ||
| math::AcosOp, math::AsinOp, math::AbsFOp, math::PowFOp, | ||
| math::FPowIOp, math::IPowIOp>(); |
There was a problem hiding this comment.
Remove the integer variants here too, please.
|
|
||
| if (failed(applyPartialConversion(moduleOp, target, std::move(patterns)))) | ||
| signalPassFailure(); | ||
| } |
| #include "mlir/Conversion/Passes.h.inc" | ||
| } // namespace mlir | ||
|
|
||
| #endif // MLIR_CONVERSION_MATHTOEMITC_MATHTOEMITCPASS_H |
| This pass converts supported Math ops to call_opaque calls to compiler generated | ||
| functions implementing these operations in software. |
There was a problem hiding this comment.
nit: Maybe something like:
This pass converts supported Math ops to opaque_call ops targeting libc/libm functions.
There was a problem hiding this comment.
I changed to what you suggested.
We might add new targets as suggested in this thread
| template <typename OpType> | ||
| LogicalResult LowerToEmitCCallOpaque<OpType>::matchAndRewrite( | ||
| OpType op, PatternRewriter &rewriter) const { | ||
| auto actualOp = mlir::cast<OpType>(op); |
There was a problem hiding this comment.
Why do we need this cast?
There was a problem hiding this comment.
The cast is redundant here, I removed it
| @@ -0,0 +1,133 @@ | |||
| // RUN: mlir-opt --split-input-file -convert-math-to-emitc -verify-diagnostics %s | FileCheck %s | |||
There was a problem hiding this comment.
Split this into math-to-emitc.mlir for positive tests and math-to-emitc-failed.mlir for negative tests. You can then drop --split-input-file, verify-diagnostics and the // ----- separators from the positve tests.
There was a problem hiding this comment.
Added math-to-emitc-failed.mlir which tests invalid types.
| // CHECK: return | ||
| // CHECK: } | ||
| func.func @absf_to_call_opaque(%arg0: f32) { | ||
| %1 = math.absf %arg0 : f32 |
There was a problem hiding this comment.
Add the f64 variants for the tests please
There was a problem hiding this comment.
I decided to support only float32 until I decide how to proceed with this thread. I would appreciate you opinion about it.
There was a problem hiding this comment.
Added a commit that support both f64 and f32, and updated the lit tests
There was a problem hiding this comment.
Can you make the tests exhaustive, with something like this:
func.func @floor(%arg0: f32, %arg1: f64) {
// C: emitc.call_opaque "floorf" (%arg0)
// C-NEXT: emitc.call_opaque "floor" (%arg1)
// CPP: emitc.call_opaque "std::floor" (%arg0)
// CPP-NEXT: emitc.call_opaque "std::floor" (%arg1)
%0 = math.floor %arg0 : f32
%1 = math.floor %arg1 : f64
return
}Addiotionally a test for math.round is missing currently.
There was a problem hiding this comment.
Looks good. I changed the test accordingly and added one for math.round.
| func.func @test(%arg0 : tensor<4xf32>) -> tensor<4xf32> { | ||
| // expected-error @+2 {{failed to legalize operation 'math.absf' that was explicitly marked illegal}} | ||
| // expected-error @+1 {{non-float types are not supported}} | ||
| %0 = math.absf %arg0 : tensor<4xf32> |
There was a problem hiding this comment.
Can you add tests for unsupported bit-widths, like f16, f128
There was a problem hiding this comment.
I added those to math-to-emitc-failed.mlir
| LogicalResult LowerToEmitCCallOpaque<OpType>::matchAndRewrite( | ||
| OpType op, PatternRewriter &rewriter) const { | ||
| auto actualOp = mlir::cast<OpType>(op); | ||
| if (!llvm::all_of( |
There was a problem hiding this comment.
You can use something like
!llvm::all_of(actualOp->getOperandTypes(), llvm::IsaPred<Float32Type, Float64Type>)There was a problem hiding this comment.
I did, thanks!
|
|
||
| namespace { | ||
| template <typename OpType> | ||
| class LowerToEmitCCallOpaque : public mlir::OpRewritePattern<OpType> { |
There was a problem hiding this comment.
Drop all the mlir:: namespaces.
There was a problem hiding this comment.
Thanks, Deleted
There was a problem hiding this comment.
I do not yet have a clear opinion on the two options you proposed so far and only a light tendency to option 1 (don't know if there is real interest in option 2 out there and we mostly implement things we see a clear demand for).
Thanks for iterating on the code and already incorporating my prior suggestions! However, I hope you don't mind that I have some more ;)
recursion-man
force-pushed
the
create-math-to-emitc-pass
branch
from
d8f0ce9 to
7c8630f
Compare
…nd restrict to f32 only Refactored code (added newlines and nits). Added tests to verify behavior with unsupported types. Now only f32 is supported. Deleted generation of emitc.include Changed the conversion to apply at the operation level instead of the module level.
recursion-man
force-pushed
the
create-math-to-emitc-pass
branch
from
7c8630f to
f6c2406
Compare
| #include "mlir/Conversion/Passes.h.inc" | ||
| } // namespace mlir | ||
|
|
||
| #endif // MLIR_CONVERSION_MATHTOEMITC_MATHTOEMITCPASS_H |
| This pass converts supported Math ops to call_opaque calls to compiler generated | ||
| functions implementing these operations in software. |
There was a problem hiding this comment.
I changed to what you suggested.
We might add new targets as suggested in this thread
|
|
||
| namespace { | ||
| template <typename OpType> | ||
| class LowerToEmitCCallOpaque : public mlir::OpRewritePattern<OpType> { |
There was a problem hiding this comment.
Thanks, Deleted
| template <typename OpType> | ||
| LogicalResult LowerToEmitCCallOpaque<OpType>::matchAndRewrite( | ||
| OpType op, PatternRewriter &rewriter) const { | ||
| auto actualOp = mlir::cast<OpType>(op); |
There was a problem hiding this comment.
The cast is redundant here, I removed it
| LogicalResult LowerToEmitCCallOpaque<OpType>::matchAndRewrite( | ||
| OpType op, PatternRewriter &rewriter) const { | ||
| auto actualOp = mlir::cast<OpType>(op); | ||
| if (!llvm::all_of( |
There was a problem hiding this comment.
I did, thanks!
| target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundEvenOp, | ||
| math::CosOp, math::SinOp, math::Atan2Op, math::CeilOp, | ||
| math::AcosOp, math::AsinOp, math::AbsFOp, math::PowFOp, | ||
| math::FPowIOp, math::IPowIOp>(); |
| @@ -0,0 +1,133 @@ | |||
| // RUN: mlir-opt --split-input-file -convert-math-to-emitc -verify-diagnostics %s | FileCheck %s | |||
There was a problem hiding this comment.
Added math-to-emitc-failed.mlir which tests invalid types.
| // CHECK: return | ||
| // CHECK: } | ||
| func.func @absf_to_call_opaque(%arg0: f32) { | ||
| %1 = math.absf %arg0 : f32 |
There was a problem hiding this comment.
I decided to support only float32 until I decide how to proceed with this thread. I would appreciate you opinion about it.
| func.func @test(%arg0 : tensor<4xf32>) -> tensor<4xf32> { | ||
| // expected-error @+2 {{failed to legalize operation 'math.absf' that was explicitly marked illegal}} | ||
| // expected-error @+1 {{non-float types are not supported}} | ||
| %0 = math.absf %arg0 : tensor<4xf32> |
There was a problem hiding this comment.
I added those to math-to-emitc-failed.mlir
| // expected-error @+1 {{non-float types are not supported}} | ||
| %0 = math.absf %arg0 : tensor<4xf32> | ||
| return %0 : tensor<4xf32> | ||
| } |
recursion-man
force-pushed
the
create-math-to-emitc-pass
branch
from
6743eea to
23ada46
Compare
|
Hi @marbre, @simon-camp, @cferry-AMD I’ve made some updates to the PR, specifically around the decision logic for target support. Now, the changes determine whether we support C or C++ as the target language. Could you please review the latest modifications when you have a chance? Thank you! |
|
Looks good to me |
There was a problem hiding this comment.
Thanks for further iterating on this. I am currently quite busy and only have limited time for reviews. So please excuse any (future) delays and don't take is as a lack of interest (it is not!).
With the current PR we have "C" and "CPP" as possible targets (I actually would prefer lowercase here). What about specifying language standard instead? While not having a demand myself right now, I think adding the option to add c23 to e.g. emit fabsd32 could be nice (?). Any (further) opinions on this?
| def MathToEmitCLanguageTarget : I32EnumAttr<"MathToEmitCLanguageTarget", | ||
| "Specifies the language target for generating callees.", [ | ||
| I32EnumAttrCase<"C", 0, "Use C-style function names">, | ||
| I32EnumAttrCase<"CPP", 1, "Use C++-style function names"> | ||
| ]> { | ||
| let cppNamespace = "::mlir::emitc"; | ||
| } |
There was a problem hiding this comment.
I don't think this should be placed in EmitCAttributes.td as it is not strictly belongs to the dialect itself. Scanning over upstream quickly it seems that ConvertSPIRVToLLVMPass relies on an Option which uses a custom attribute and not one of the standard types. As another example, include/mlir/Dialect/ArmSME/Transforms/Passes.td defines attributes in the passes file itself. I don't know the best solution for this but as mentioned EmitCAttributes.td feels like the wrong place.
There was a problem hiding this comment.
I wonder if we could take advantage of this use case to introduce a more general attribute attached to the module, e.g. EmitCTarget/EmitCTargetLanguage, to be used consistently by lowering passes and the translator where distinct C variants require different lowering, module-level verification or syntax, e.g.
- Multiple return values are currently lowered to C++ has
std::tuple, unavailable in other C variants. - OpenCL-C defines several address spaces, other C variants don't.
- OpenCL-C has native vector types (to some extent), C doesn't.
- C++ has templates, other C variants don't.
- C/C++ support function pointers and recursion, OpenCL-C doesn't.
An alternative to such an attribute might be target-language module ops, e.g. EmitC::C99ModuleOp.
With the current PR we have "C" and "CPP" as possible targets (I actually would prefer lowercase here). What about >specifying language standard instead? While not having a demand myself right now, I think adding the option to add c23 to e.g. emit fabsd32 could be nice (?). Any (further) opinions on this?
+1
Would be good to enumerate all standard C/C++ versions and let each pass differentiate its lowering as needed.
There was a problem hiding this comment.
Thanks for the feedback!
@marbre, I’m waiting to hear your opinion on @aniragil's suggestion to use a module-level attribute.
As for C23, since decimal types aren’t yet in the builtin types, lowering into them could be challenging. If C23 doesn’t add immediate benefits, maybe we can start with c99 and cpp11. Thoughts?"
There was a problem hiding this comment.
No need to ass C23 from the start but I would prefer a naming (c99, cpp11, ...) that allows to extend if we want to (we would be a little stuck with just CPP and C).
Let me try to also pull in the other parallel discussion. With regards to @cferry-AMD's comment
How specific to Math functions do you see this pass? The second choice makes the lowering much more generic than I thought: it may apply to any op that could be lowered to opaque library calls, and not just Math dialect ops to
math.hfunctions. I like this ability to lower anything to opaque calls with language filters, but this may not be what you intended.
and your response
Thank you for the feedback! I intended this pass to focus on lowering math functions specifically. I accepted the suggestion to use libraries like and am also considering the option to add custom math libraries, not just the standard ones, which motivated me to use a map. I can enforce that only math dialect ops will be converted to ensure it remains targeted.
I feel like shouldn't put to much into this single PR. We can still use what you have to iterate on it under the assumption that we don't land an approach that introduces drawbacks (for example ops, parameters, ..) that will break with the first refactoring commit and thus break users. Having something that can be consistently used in multiple conversions could be nice, but I currently don't have a concrete idea of how this could look like and lack time to spend much though on it. It would indeed be nice to solve some of the some of issues to which @aniragil pointed but I don't know if we can solve all these with this kind of the proposed attribute. Target module ops like EmitC::C99ModuleOp can be an alternative but are a different concept. Spontaneously, I would prefer to just go a different conversion route (which is what you're doing with the proposed patch) than introducing completely new ops.
There was a problem hiding this comment.
@marbre, thanks for the input.
I noticed that ConvertSPIRVToLLVMPass relies on a custom Option for ClientAPI defined in SPIRVBase.td, which is similar to what I’m aiming for here. Additionally, include/mlir/Dialect/ArmSME/Transforms/Passes.td defines attributes directly in its passes file but includes mlir/IR/EnumAttr.td, going beyond the usual Conversion/Passes.td convention to include only mlir/Pass/PassBase.td.
At this point, defining the attribute in EmitCBase.td or EmitCAttributes.td seems like the only alternative I can think of. What are your thoughts?
I also agree with your point on keeping this PR focused without adding too much at once. Further ideas we discussed can be part of future work.
There was a problem hiding this comment.
Does this have to be an attribute right now? As we don't have a dedicated op we could attach this to yet, we could start with a LanguageTarget enum somewhere in Dialect/EmitC/Transforms. Once we introduce a module like op we can give it an inherent attribute to define the target.
Additionally I would strip the Math prefix from the name as I expect this to be used in more places in the future as @aniragil mentioned.
There was a problem hiding this comment.
Sounds about right to me. I've moved the LanguageTarget enum to Dialect/EmitC/Transforms/Passes.h as you suggested and removed the Math prefix to generalize the enum for broader use.
| if (!llvm::all_of(op->getOperandTypes(), llvm::IsaPred<Float32Type, Float64Type>)|| | ||
| !llvm::all_of(op->getResultTypes(),llvm::IsaPred<Float32Type, Float64Type>)) | ||
| return rewriter.notifyMatchFailure( | ||
| op.getLoc(), "expected all operands and results to be of type f32"); |
There was a problem hiding this comment.
| op.getLoc(), "expected all operands and results to be of type f32"); | |
| op.getLoc(), "expected all operands and results to be of type f32 or f64"); |
| def MathToEmitCLanguageTarget : I32EnumAttr<"MathToEmitCLanguageTarget", | ||
| "Specifies the language target for generating callees.", [ | ||
| I32EnumAttrCase<"C", 0, "Use C-style function names">, | ||
| I32EnumAttrCase<"CPP", 1, "Use C++-style function names"> | ||
| ]> { | ||
| let cppNamespace = "::mlir::emitc"; | ||
| } |
There was a problem hiding this comment.
Does this have to be an attribute right now? As we don't have a dedicated op we could attach this to yet, we could start with a LanguageTarget enum somewhere in Dialect/EmitC/Transforms. Once we introduce a module like op we can give it an inherent attribute to define the target.
Additionally I would strip the Math prefix from the name as I expect this to be used in more places in the future as @aniragil mentioned.
| ConversionTarget target(getContext()); | ||
| target.addLegalOp<emitc::CallOpaqueOp>(); | ||
|
|
||
| target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundEvenOp, |
There was a problem hiding this comment.
| target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundEvenOp, | |
| target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundOp, |
| // CHECK: return | ||
| // CHECK: } | ||
| func.func @absf_to_call_opaque(%arg0: f32) { | ||
| %1 = math.absf %arg0 : f32 |
There was a problem hiding this comment.
Can you make the tests exhaustive, with something like this:
func.func @floor(%arg0: f32, %arg1: f64) {
// C: emitc.call_opaque "floorf" (%arg0)
// C-NEXT: emitc.call_opaque "floor" (%arg1)
// CPP: emitc.call_opaque "std::floor" (%arg0)
// CPP-NEXT: emitc.call_opaque "std::floor" (%arg1)
%0 = math.floor %arg0 : f32
%1 = math.floor %arg1 : f64
return
}Addiotionally a test for math.round is missing currently.
| ConversionTarget target(getContext()); | ||
| target.addLegalOp<emitc::CallOpaqueOp>(); | ||
|
|
||
| target.addIllegalOp<math::FloorOp, math::ExpOp, math::RoundEvenOp, |
| if (!llvm::all_of(op->getOperandTypes(), llvm::IsaPred<Float32Type, Float64Type>)|| | ||
| !llvm::all_of(op->getResultTypes(),llvm::IsaPred<Float32Type, Float64Type>)) | ||
| return rewriter.notifyMatchFailure( | ||
| op.getLoc(), "expected all operands and results to be of type f32"); |
| def MathToEmitCLanguageTarget : I32EnumAttr<"MathToEmitCLanguageTarget", | ||
| "Specifies the language target for generating callees.", [ | ||
| I32EnumAttrCase<"C", 0, "Use C-style function names">, | ||
| I32EnumAttrCase<"CPP", 1, "Use C++-style function names"> | ||
| ]> { | ||
| let cppNamespace = "::mlir::emitc"; | ||
| } |
There was a problem hiding this comment.
Sounds about right to me. I've moved the LanguageTarget enum to Dialect/EmitC/Transforms/Passes.h as you suggested and removed the Math prefix to generalize the enum for broader use.
| // CHECK: return | ||
| // CHECK: } | ||
| func.func @absf_to_call_opaque(%arg0: f32) { | ||
| %1 = math.absf %arg0 : f32 |
There was a problem hiding this comment.
Looks good. I changed the test accordingly and added one for math.round.
|
Hi @marbre and @simon-camp, I hope you're doing well. I wanted to follow up on my MathToEmitC PR. I’ve been away for the past couple of months due to personal commitments, but I’m now back and ready to move this forward. Thanks for your patience! Here’s a quick summary of the changes since we last discussed: Following @simon-camp’s suggestion, I replaced the MathToEmitCLanguageTarget attribute with a LanguageTarget enum in Dialect/EmitC/Transforms/Passes.h. The enum now supports c99 and cpp11 for better precision. I’d appreciate it if you could take another look and let me know if there’s anything else I should address. |
recursion-man
force-pushed
the
create-math-to-emitc-pass
branch
from
b1ab280 to
e98c7f5
Compare
recursion-man
force-pushed
the
create-math-to-emitc-pass
branch
from
e98c7f5 to
e6b8175
Compare
…et enum class - Created LanguageTarget enum class in MathToEmitC.h. An enum that specifies the language target for code generation. - Added language standard target option (c99, cpp11): allowing users to choose between C99 and C++11.
recursion-man
force-pushed
the
create-math-to-emitc-pass
branch
from
e6b8175 to
8992fd5
Compare
|
Before we can merge, can you please update the PR description to reflect the current state? The PR description will become the body of the commit message when squashing. https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch
Thanks in advance! |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Seems there are also some formatting changes necessary. I didn't realized that the CI needed approval to fully run. |
|
Hi @marbre and @simon-camp, Thanks again for your reviews and support! All checks have passed, and the PR is ready to merge. Could you approve the pending workflow and merge it? Appreciate it! |
|
@recursion-man Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Merged, thanks again for your patch! |
This commit introduces a new MathToEmitC conversion pass that lowers selected math operations from the Math dialect to the emitc.call_opaque operation in the EmitC dialect.
Supported Math Operations:
The following operations are converted:
Target Language Standards:
The pass supports targeting different language standards:
Design Decisions:
The pass uses emitc.call_opaque instead of emitc.call to better emulate C-style function overloading.
emitc.call_opaque does not require a unique type signature, making it more suitable for operations like <math.h> functions that may be overloaded for different types.
This design choice ensures compatibility with C/C++ conventions.