[mlir][math] Replace roundeven call by nearbyint call when target does not have C23 features #88687
Conversation
[FXML-4442] Roundeven is only C23, emit nearbyint otherwise
|
@llvm/pr-subscribers-flang-codegen @llvm/pr-subscribers-mlir Author: Corentin Ferry (cferry-AMD) ChangesThis commit:
Full diff: https://github.com/llvm/llvm-project/pull/88687.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Conversion/MathToLibm/MathToLibm.h b/mlir/include/mlir/Conversion/MathToLibm/MathToLibm.h
index ab9a1cef20cab7..1fac96abef621f 100644
--- a/mlir/include/mlir/Conversion/MathToLibm/MathToLibm.h
+++ b/mlir/include/mlir/Conversion/MathToLibm/MathToLibm.h
@@ -19,10 +19,13 @@ class OperationPass;
/// Populate the given list with patterns that convert from Math to Libm calls.
/// If log1pBenefit is present, use it instead of benefit for the Log1p op.
-void populateMathToLibmConversionPatterns(RewritePatternSet &patterns);
+void populateMathToLibmConversionPatterns(
+ RewritePatternSet &patterns, const ConvertMathToLibmOptions &options);
/// Create a pass to convert Math operations to libm calls.
std::unique_ptr<OperationPass<ModuleOp>> createConvertMathToLibmPass();
+std::unique_ptr<OperationPass<ModuleOp>>
+createConvertMathToLibmPass(const ConvertMathToLibmOptions &options);
} // namespace mlir
diff --git a/mlir/include/mlir/Conversion/Passes.td b/mlir/include/mlir/Conversion/Passes.td
index d094ee3b36ab95..8f8eacdb6e8c1c 100644
--- a/mlir/include/mlir/Conversion/Passes.td
+++ b/mlir/include/mlir/Conversion/Passes.td
@@ -692,6 +692,12 @@ def ConvertMathToLibm : Pass<"convert-math-to-libm", "ModuleOp"> {
"func::FuncDialect",
"vector::VectorDialect",
];
+ let options = [
+ Option<"allowC23Features", "allow-c23-features", "bool", "true",
+ "Allow calls to C23-specific functions">,
+ Option<"roundingModeIsDefault", "rounding-mode-is-default", "bool", "false",
+ "Assume default rounding mode">
+ ];
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp b/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
index 5b1c59d0c95e92..1c500b397edd4c 100644
--- a/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
+++ b/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
@@ -159,7 +159,8 @@ ScalarOpToLibmCall<Op>::matchAndRewrite(Op op,
return success();
}
-void mlir::populateMathToLibmConversionPatterns(RewritePatternSet &patterns) {
+void mlir::populateMathToLibmConversionPatterns(
+ RewritePatternSet &patterns, const ConvertMathToLibmOptions &options) {
MLIRContext *ctx = patterns.getContext();
populatePatternsForOp<math::AbsFOp>(patterns, ctx, "fabsf", "fabs");
@@ -185,8 +186,15 @@ void mlir::populateMathToLibmConversionPatterns(RewritePatternSet &patterns) {
populatePatternsForOp<math::Log10Op>(patterns, ctx, "log10f", "log10");
populatePatternsForOp<math::Log1pOp>(patterns, ctx, "log1pf", "log1p");
populatePatternsForOp<math::PowFOp>(patterns, ctx, "powf", "pow");
- populatePatternsForOp<math::RoundEvenOp>(patterns, ctx, "roundevenf",
- "roundeven");
+ if (options.allowC23Features)
+ populatePatternsForOp<math::RoundEvenOp>(patterns, ctx, "roundevenf",
+ "roundeven");
+ else if (options.roundingModeIsDefault)
+ populatePatternsForOp<math::RoundEvenOp>(patterns, ctx, "nearbyintf",
+ "nearbyint");
+ // Roundeven: using nearbyint (pre-C23) for roundeven requires the
+ // rounding mode to be FE_TONEAREST (the default). Otherwise we need to
+ // issue a call to set the rounding mode (which this pass currently can't do).
populatePatternsForOp<math::RoundOp>(patterns, ctx, "roundf", "round");
populatePatternsForOp<math::SinOp>(patterns, ctx, "sinf", "sin");
populatePatternsForOp<math::SinhOp>(patterns, ctx, "sinhf", "sinh");
@@ -199,6 +207,7 @@ void mlir::populateMathToLibmConversionPatterns(RewritePatternSet &patterns) {
namespace {
struct ConvertMathToLibmPass
: public impl::ConvertMathToLibmBase<ConvertMathToLibmPass> {
+ using Base::Base;
void runOnOperation() override;
};
} // namespace
@@ -207,7 +216,9 @@ void ConvertMathToLibmPass::runOnOperation() {
auto module = getOperation();
RewritePatternSet patterns(&getContext());
- populateMathToLibmConversionPatterns(patterns);
+ populateMathToLibmConversionPatterns(
+ patterns, {/*allowC23Features=*/allowC23Features,
+ /*roundingModeIsDefault=*/roundingModeIsDefault});
ConversionTarget target(getContext());
target.addLegalDialect<arith::ArithDialect, BuiltinDialect, func::FuncDialect,
@@ -220,3 +231,8 @@ void ConvertMathToLibmPass::runOnOperation() {
std::unique_ptr<OperationPass<ModuleOp>> mlir::createConvertMathToLibmPass() {
return std::make_unique<ConvertMathToLibmPass>();
}
+
+std::unique_ptr<OperationPass<ModuleOp>>
+mlir::createConvertMathToLibmPass(const ConvertMathToLibmOptions &options) {
+ return std::make_unique<ConvertMathToLibmPass>(options);
+}
diff --git a/mlir/test/Conversion/MathToLibm/nearbyint.mlir b/mlir/test/Conversion/MathToLibm/nearbyint.mlir
new file mode 100644
index 00000000000000..82d03dab5d985e
--- /dev/null
+++ b/mlir/test/Conversion/MathToLibm/nearbyint.mlir
@@ -0,0 +1,41 @@
+// RUN: mlir-opt %s --pass-pipeline='builtin.module(convert-math-to-libm{allow-c23-features=0 rounding-mode-is-default}, canonicalize)' | FileCheck %s
+
+// CHECK-DAG: @nearbyint(f64) -> f64 attributes {libm, llvm.readnone}
+// CHECK-DAG: @nearbyintf(f32) -> f32 attributes {libm, llvm.readnone}
+
+// CHECK-LABEL: func @nearbyint_caller
+// CHECK-SAME: %[[FLOAT:.*]]: f32
+// CHECK-SAME: %[[DOUBLE:.*]]: f64
+func.func @nearbyint_caller(%float: f32, %double: f64) -> (f32, f64) {
+ // CHECK-DAG: %[[FLOAT_RESULT:.*]] = call @nearbyintf(%[[FLOAT]]) : (f32) -> f32
+ %float_result = math.roundeven %float : f32
+ // CHECK-DAG: %[[DOUBLE_RESULT:.*]] = call @nearbyint(%[[DOUBLE]]) : (f64) -> f64
+ %double_result = math.roundeven %double : f64
+ // CHECK: return %[[FLOAT_RESULT]], %[[DOUBLE_RESULT]]
+ return %float_result, %double_result : f32, f64
+}
+
+// CHECK-LABEL: func @nearbyint_vec_caller(
+// CHECK-SAME: %[[VAL_0:.*]]: vector<2xf32>,
+// CHECK-SAME: %[[VAL_1:.*]]: vector<2xf64>) -> (vector<2xf32>, vector<2xf64>) {
+// CHECK-DAG: %[[CVF:.*]] = arith.constant dense<0.000000e+00> : vector<2xf32>
+// CHECK-DAG: %[[CVD:.*]] = arith.constant dense<0.000000e+00> : vector<2xf64>
+// CHECK: %[[IN0_F32:.*]] = vector.extract %[[VAL_0]][0] : f32 from vector<2xf32>
+// CHECK: %[[OUT0_F32:.*]] = call @nearbyintf(%[[IN0_F32]]) : (f32) -> f32
+// CHECK: %[[VAL_8:.*]] = vector.insert %[[OUT0_F32]], %[[CVF]] [0] : f32 into vector<2xf32>
+// CHECK: %[[IN1_F32:.*]] = vector.extract %[[VAL_0]][1] : f32 from vector<2xf32>
+// CHECK: %[[OUT1_F32:.*]] = call @nearbyintf(%[[IN1_F32]]) : (f32) -> f32
+// CHECK: %[[VAL_11:.*]] = vector.insert %[[OUT1_F32]], %[[VAL_8]] [1] : f32 into vector<2xf32>
+// CHECK: %[[IN0_F64:.*]] = vector.extract %[[VAL_1]][0] : f64 from vector<2xf64>
+// CHECK: %[[OUT0_F64:.*]] = call @nearbyint(%[[IN0_F64]]) : (f64) -> f64
+// CHECK: %[[VAL_14:.*]] = vector.insert %[[OUT0_F64]], %[[CVD]] [0] : f64 into vector<2xf64>
+// CHECK: %[[IN1_F64:.*]] = vector.extract %[[VAL_1]][1] : f64 from vector<2xf64>
+// CHECK: %[[OUT1_F64:.*]] = call @nearbyint(%[[IN1_F64]]) : (f64) -> f64
+// CHECK: %[[VAL_17:.*]] = vector.insert %[[OUT1_F64]], %[[VAL_14]] [1] : f64 into vector<2xf64>
+// CHECK: return %[[VAL_11]], %[[VAL_17]] : vector<2xf32>, vector<2xf64>
+// CHECK: }
+func.func @nearbyint_vec_caller(%float: vector<2xf32>, %double: vector<2xf64>) -> (vector<2xf32>, vector<2xf64>) {
+ %float_result = math.roundeven %float : vector<2xf32>
+ %double_result = math.roundeven %double : vector<2xf64>
+ return %float_result, %double_result : vector<2xf32>, vector<2xf64>
+}
diff --git a/mlir/test/Conversion/MathToLibm/roundeven-failed.mlir b/mlir/test/Conversion/MathToLibm/roundeven-failed.mlir
new file mode 100644
index 00000000000000..d7aa976a496ba6
--- /dev/null
+++ b/mlir/test/Conversion/MathToLibm/roundeven-failed.mlir
@@ -0,0 +1,8 @@
+// RUN: mlir-opt %s --pass-pipeline='builtin.module(convert-math-to-libm{allow-c23-features=0 rounding-mode-is-default=0})' -verify-diagnostics
+
+func.func @nearbyint_caller(%float: f32, %double: f64) -> (f32, f64) {
+ // expected-error@+1 {{failed to legalize operation 'math.roundeven'}}
+ %float_result = math.roundeven %float : f32
+ %double_result = math.roundeven %double : f64
+ return %float_result, %double_result : f32, f64
+}
|
|
For review, let's call @marbre @simon-camp @mgehre . |
|
@TinaAMD , can you review here as well? |
llvmbot
added
flang
|
@frederik-h, you were suggested to me as a reviewer for this PR :) |
|
@cferry-AMD I added the reviewers you asked for but it seems this is not related to EmitC at all. Thus we can review but I think there are other code owners that should definitely look into this as well! Though, haven't checked who should take a look to give an final approval. |
| else if (options.roundingModeIsDefault) | ||
| populatePatternsForOp<math::RoundEvenOp>(patterns, ctx, "nearbyintf", | ||
| "nearbyint"); | ||
| // Roundeven: using nearbyint (pre-C23) for roundeven requires the |
There was a problem hiding this comment.
I am not really qualified to review those changes, but since you asked for a review, perhaps it helps if I ask a few questions to understand what you are trying to do. I see that you need this for Flang. I suppose that roundeven provides the semantics that you need for Fortran. How is this related to "C23" features? Isn't this a question about what is implemented in your libm? It seems that glibc has implemented roundeven for several years, see here. Is it unimplemented on any target that you need?
There was a problem hiding this comment.
Thanks @frederik-h. I have touched flang code at some point in an attempt to fix a compile-time bug that went unnoticed on my end, but went backwards and found out I could fix it at the MathToLibm end. So this has nothing to do with Flang... sorry for the noise.
My use case is indeed a C compiler that does not support (yet) roundeven. It comes with its own libm implementation that does not feature this function.
There was a problem hiding this comment.
Thanks for the explanation. So I assume that using nearbyint instead of roundeven should be equivalent as long as no one changes the rounding mode? Or are there other semantics differences? I am wondering if you could handle this in your frontend if there was a nearbyint in the math dialect? Not sure if that's better though.
There was a problem hiding this comment.
My use case is indeed a C compiler that does not support (yet) roundeven. It comes with its own libm implementation that does not feature this function.
So this is a workaround for a missing implementation of roundeven in your libm? Some thoughts about alternative solutions: (1) Implement it in your libm, (2) keep this change in your projects fork of mlir until you implement it if that exists, or (3) add a math.nearbyint and document that its behavior depends on a target specific rounding mode (assuming that this would be at least as acceptable for the math dialect as the changes from this PR) and then replace math.roundeven by that in a pass in your front end? (3) sounds like the best approach to me assuming that (1) is not an option for you right now, but we would need the opinion of someone who is more familiar with the design goals of the dialect.
There was a problem hiding this comment.
Thanks for your insight. I unfortunately cannot do option (1); option (3) would result in having two very similar operations in math. Option (2) is feasible.
Currently, MathToLibm is tied to specific versions of libm that feature the roundeven function; to me, it would make sense to be able to specify which standard / version of libm is targeted so that only compatible calls are issued. There would need to be more use cases to this than just mine to warrant such a big change though.
| @@ -185,8 +186,15 @@ void mlir::populateMathToLibmConversionPatterns(RewritePatternSet &patterns) { | |||
| populatePatternsForOp<math::Log10Op>(patterns, ctx, "log10f", "log10"); | |||
| populatePatternsForOp<math::Log1pOp>(patterns, ctx, "log1pf", "log1p"); | |||
| populatePatternsForOp<math::PowFOp>(patterns, ctx, "powf", "pow"); | |||
| populatePatternsForOp<math::RoundEvenOp>(patterns, ctx, "roundevenf", | |||
| "roundeven"); | |||
| if (options.allowC23Features) | |||
There was a problem hiding this comment.
Could there be a better name for this? Not every user of the math dialect is a C frontend.
There was a problem hiding this comment.
Right, as also suggested by the flang issue raised earlier. So maybe an allow-roundeven flag? I'm not keen on adding a flag this specific, but if nothing else in MathToLibm appears to need a flag, that could just be it.
| if (options.allowC23Features) | ||
| populatePatternsForOp<math::RoundEvenOp>(patterns, ctx, "roundevenf", | ||
| "roundeven"); | ||
| else if (options.roundingModeIsDefault) |
There was a problem hiding this comment.
What about the else case?
There was a problem hiding this comment.
The "else" case is not implemented in this commit, so we don't lowering the roundeven operation. There, the rounding mode is not assumed to be "round to nearest", and so it would need to be set accordingly if we want to use nearbyint.
I'm thinking of a call to fegetround to get the current rounding mode, fesetround to set it if necessary, and another fesetround to restore the previous rounding mode, but this may not be the right approach (makes the generated code use side effects, and may slow it down).
|
There does not look like to be enough use cases at this point of another lowering of |
This commit:
math.roundevento calls toroundevenonly if C23 features are allowed,FE_TONEAREST) (default: no),math.roundevento calls tonearbyint.A bit more details: This commit introduces a flag to "allow a certain standard", which I'm not a fan of because that could lead to a multiplication of flags specific to each pass and each standard. Ideally it would piggyback on some flag that states which language/standard is the target. What are your thoughts on this?