-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[mlir] [math] Fix the precision issue of expand math #120865
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The convertFloorOp pattern incurs precision loss when floating-point numbers exceed the representable range of int64. This pattern should be removed. Fixes llvm#119836
Member
|
@llvm/pr-subscribers-mlir-math @llvm/pr-subscribers-mlir Author: donald chen (cxy-1993) ChangesThe convertFloorOp pattern incurs precision loss when floating-point numbers exceed the representable range of int64. This pattern should be removed. Fixes #119836 Full diff: https://github.com/llvm/llvm-project/pull/120865.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Math/Transforms/Passes.h b/mlir/include/mlir/Dialect/Math/Transforms/Passes.h
index 74ba91322ea857..f0f17c6adcb088 100644
--- a/mlir/include/mlir/Dialect/Math/Transforms/Passes.h
+++ b/mlir/include/mlir/Dialect/Math/Transforms/Passes.h
@@ -31,7 +31,6 @@ void populateExpandAsinhPattern(RewritePatternSet &patterns);
void populateExpandAcoshPattern(RewritePatternSet &patterns);
void populateExpandAtanhPattern(RewritePatternSet &patterns);
void populateExpandFmaFPattern(RewritePatternSet &patterns);
-void populateExpandFloorFPattern(RewritePatternSet &patterns);
void populateExpandCeilFPattern(RewritePatternSet &patterns);
void populateExpandExp2FPattern(RewritePatternSet &patterns);
void populateExpandPowFPattern(RewritePatternSet &patterns);
diff --git a/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp b/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
index 8bcbdb4c9a664a..3dadf9474cf4f6 100644
--- a/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
+++ b/mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
@@ -215,31 +215,6 @@ static LogicalResult convertFmaFOp(math::FmaOp op, PatternRewriter &rewriter) {
return success();
}
-// Converts a floorf() function to the following:
-// floorf(float x) ->
-// y = (float)(int) x
-// if (x < 0) then incr = -1 else incr = 0
-// y = y + incr <= replace this op with the floorf op.
-static LogicalResult convertFloorOp(math::FloorOp op,
- PatternRewriter &rewriter) {
- ImplicitLocOpBuilder b(op->getLoc(), rewriter);
- Value operand = op.getOperand();
- Type opType = operand.getType();
- Value fpFixedConvert = createTruncatedFPValue(operand, b);
-
- // Creating constants for later use.
- Value zero = createFloatConst(op->getLoc(), opType, 0.00, rewriter);
- Value negOne = createFloatConst(op->getLoc(), opType, -1.00, rewriter);
-
- Value negCheck =
- b.create<arith::CmpFOp>(arith::CmpFPredicate::OLT, operand, zero);
- Value incrValue =
- b.create<arith::SelectOp>(op->getLoc(), negCheck, negOne, zero);
- Value ret = b.create<arith::AddFOp>(opType, fpFixedConvert, incrValue);
- rewriter.replaceOp(op, ret);
- return success();
-}
-
// Converts a ceilf() function to the following:
// ceilf(float x) ->
// y = (float)(int) x
@@ -696,10 +671,6 @@ void mlir::populateExpandRoundFPattern(RewritePatternSet &patterns) {
patterns.add(convertRoundOp);
}
-void mlir::populateExpandFloorFPattern(RewritePatternSet &patterns) {
- patterns.add(convertFloorOp);
-}
-
void mlir::populateExpandRoundEvenPattern(RewritePatternSet &patterns) {
patterns.add(convertRoundEvenOp);
}
diff --git a/mlir/test/Dialect/Math/expand-math.mlir b/mlir/test/Dialect/Math/expand-math.mlir
index 89413b95703322..6055ed0504c84c 100644
--- a/mlir/test/Dialect/Math/expand-math.mlir
+++ b/mlir/test/Dialect/Math/expand-math.mlir
@@ -133,24 +133,6 @@ func.func @fmaf_func(%a: f64, %b: f64, %c: f64) -> f64 {
// -----
-// CHECK-LABEL: func @floorf_func
-// CHECK-SAME: ([[ARG0:%.+]]: f64) -> f64
-func.func @floorf_func(%a: f64) -> f64 {
- // CHECK-DAG: [[CST:%.+]] = arith.constant 0.000
- // CHECK-DAG: [[CST_0:%.+]] = arith.constant -1.000
- // CHECK-NEXT: [[CVTI:%.+]] = arith.fptosi [[ARG0]]
- // CHECK-NEXT: [[CVTF:%.+]] = arith.sitofp [[CVTI]]
- // CHECK-NEXT: [[COPYSIGN:%.+]] = math.copysign [[CVTF]], [[ARG0]]
- // CHECK-NEXT: [[COMP:%.+]] = arith.cmpf olt, [[ARG0]], [[CST]]
- // CHECK-NEXT: [[INCR:%.+]] = arith.select [[COMP]], [[CST_0]], [[CST]]
- // CHECK-NEXT: [[ADDF:%.+]] = arith.addf [[COPYSIGN]], [[INCR]]
- // CHECK-NEXT: return [[ADDF]]
- %ret = math.floor %a : f64
- return %ret : f64
-}
-
-// -----
-
// CHECK-LABEL: func @ceilf_func
// CHECK-SAME: ([[ARG0:%.+]]: f64) -> f64
func.func @ceilf_func(%a: f64) -> f64 {
diff --git a/mlir/test/lib/Dialect/Math/TestExpandMath.cpp b/mlir/test/lib/Dialect/Math/TestExpandMath.cpp
index 69af2a08b97bde..0de6d6c55a4a5e 100644
--- a/mlir/test/lib/Dialect/Math/TestExpandMath.cpp
+++ b/mlir/test/lib/Dialect/Math/TestExpandMath.cpp
@@ -46,7 +46,6 @@ void TestExpandMathPass::runOnOperation() {
populateExpandAcoshPattern(patterns);
populateExpandAtanhPattern(patterns);
populateExpandFmaFPattern(patterns);
- populateExpandFloorFPattern(patterns);
populateExpandCeilFPattern(patterns);
populateExpandPowFPattern(patterns);
populateExpandFPowIPattern(patterns);
|
Contributor
Author
|
ping @bviyer |
1 similar comment
Contributor
Author
|
ping @bviyer |
bviyer
reviewed
Jan 7, 2025
bviyer
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The convertFloorOp pattern incurs precision loss when floating-point numbers exceed the representable range of int64. This pattern should be removed.
Fixes #119836