[flang] Fixed MODULO(x, inf) to produce NaN. #86145
Merged
Conversation
This file contains 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
Straightforward computation of `A − FLOOR (A / P) * P` should produce NaN, when P is infinity. The -menable-no-infs lowering can still use the relaxed operations sequence.
llvmbot
added
flang:runtime
flang
|
@llvm/pr-subscribers-flang-runtime Author: Slava Zakharin (vzakhari) ChangesStraightforward computation of Full diff: https://github.com/llvm/llvm-project/pull/86145.diff 5 Files Affected:
diff --git a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
index eb8f5135ff12e0..88306757c2b06a 100644
--- a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
+++ b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
@@ -5259,9 +5259,12 @@ mlir::Value IntrinsicLibrary::genModulo(mlir::Type resultType,
remainder);
}
+ auto fastMathFlags = builder.getFastMathFlags();
// F128 arith::RemFOp may be lowered to a runtime call that may be unsupported
// on the target, so generate a call to Fortran Runtime's ModuloReal16.
- if (resultType == mlir::FloatType::getF128(builder.getContext()))
+ if (resultType == mlir::FloatType::getF128(builder.getContext()) ||
+ (fastMathFlags & mlir::arith::FastMathFlags::ninf) ==
+ mlir::arith::FastMathFlags::none)
return builder.createConvert(
loc, resultType,
fir::runtime::genModulo(builder, loc, args[0], args[1]));
diff --git a/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp b/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp
index 4dcbd13cb319f5..81d5d21ece7ae1 100644
--- a/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp
+++ b/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp
@@ -118,6 +118,20 @@ struct ForcedMod16 {
}
};
+/// Placeholder for real*10 version of Modulo Intrinsic
+struct ForcedModulo10 {
+ static constexpr const char *name = ExpandAndQuoteKey(RTNAME(ModuloReal10));
+ static constexpr fir::runtime::FuncTypeBuilderFunc getTypeModel() {
+ return [](mlir::MLIRContext *ctx) {
+ auto fltTy = mlir::FloatType::getF80(ctx);
+ auto strTy = fir::ReferenceType::get(mlir::IntegerType::get(ctx, 8));
+ auto intTy = mlir::IntegerType::get(ctx, 8 * sizeof(int));
+ return mlir::FunctionType::get(ctx, {fltTy, fltTy, strTy, intTy},
+ {fltTy});
+ };
+ }
+};
+
/// Placeholder for real*16 version of Modulo Intrinsic
struct ForcedModulo16 {
static constexpr const char *name = ExpandAndQuoteKey(RTNAME(ModuloReal16));
@@ -349,7 +363,13 @@ mlir::Value fir::runtime::genModulo(fir::FirOpBuilder &builder,
// MODULO is lowered into math operations in intrinsics lowering,
// so genModulo() should only be used for F128 data type now.
- if (fltTy.isF128())
+ if (fltTy.isF32())
+ func = fir::runtime::getRuntimeFunc<mkRTKey(ModuloReal4)>(loc, builder);
+ else if (fltTy.isF64())
+ func = fir::runtime::getRuntimeFunc<mkRTKey(ModuloReal8)>(loc, builder);
+ else if (fltTy.isF80())
+ func = fir::runtime::getRuntimeFunc<ForcedModulo10>(loc, builder);
+ else if (fltTy.isF128())
func = fir::runtime::getRuntimeFunc<ForcedModulo16>(loc, builder);
else
fir::intrinsicTypeTODO(builder, fltTy, loc, "MODULO");
diff --git a/flang/runtime/numeric-templates.h b/flang/runtime/numeric-templates.h
index 8ea3daaa57bcf8..3084cd42f48905 100644
--- a/flang/runtime/numeric-templates.h
+++ b/flang/runtime/numeric-templates.h
@@ -242,10 +242,8 @@ inline RT_API_ATTRS T RealMod(
IS_MODULO ? "MODULO with P==0" : "MOD with P==0");
}
if (ISNANTy<T>::compute(a) || ISNANTy<T>::compute(p) ||
- ISINFTy<T>::compute(a)) {
+ ISINFTy<T>::compute(a) || ISINFTy<T>::compute(p)) {
return QNANTy<T>::compute();
- } else if (ISINFTy<T>::compute(p)) {
- return a;
}
T aAbs{ABSTy<T>::compute(a)};
T pAbs{ABSTy<T>::compute(p)};
diff --git a/flang/test/Lower/Intrinsics/modulo.f90 b/flang/test/Lower/Intrinsics/modulo.f90
index 383cb34f83c705..ac18e59033a6b6 100644
--- a/flang/test/Lower/Intrinsics/modulo.f90
+++ b/flang/test/Lower/Intrinsics/modulo.f90
@@ -1,11 +1,13 @@
-! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s
+! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s -check-prefixes=HONORINF,ALL
+! RUN: flang-new -fc1 -menable-no-infs -emit-fir -flang-deprecated-no-hlfir %s -o - | FileCheck %s -check-prefixes=CHECK,ALL
-! CHECK-LABEL: func @_QPmodulo_testr(
-! CHECK-SAME: %[[arg0:.*]]: !fir.ref<f64>{{.*}}, %[[arg1:.*]]: !fir.ref<f64>{{.*}}, %[[arg2:.*]]: !fir.ref<f64>{{.*}}) {
+! ALL-LABEL: func @_QPmodulo_testr(
+! ALL-SAME: %[[arg0:.*]]: !fir.ref<f64>{{.*}}, %[[arg1:.*]]: !fir.ref<f64>{{.*}}, %[[arg2:.*]]: !fir.ref<f64>{{.*}}) {
subroutine modulo_testr(r, a, p)
real(8) :: r, a, p
- ! CHECK-DAG: %[[a:.*]] = fir.load %[[arg1]] : !fir.ref<f64>
- ! CHECK-DAG: %[[p:.*]] = fir.load %[[arg2]] : !fir.ref<f64>
+ ! ALL-DAG: %[[a:.*]] = fir.load %[[arg1]] : !fir.ref<f64>
+ ! ALL-DAG: %[[p:.*]] = fir.load %[[arg2]] : !fir.ref<f64>
+ ! HONORINF: %[[res:.*]] = fir.call @_FortranAModuloReal8(%[[a]], %[[p]]
! CHECK-DAG: %[[rem:.*]] = arith.remf %[[a]], %[[p]] {{.*}}: f64
! CHECK-DAG: %[[zero:.*]] = arith.constant 0.000000e+00 : f64
! CHECK-DAG: %[[remNotZero:.*]] = arith.cmpf une, %[[rem]], %[[zero]] {{.*}} : f64
@@ -15,12 +17,12 @@ subroutine modulo_testr(r, a, p)
! CHECK-DAG: %[[mustAddP:.*]] = arith.andi %[[remNotZero]], %[[signDifferent]] : i1
! CHECK-DAG: %[[remPlusP:.*]] = arith.addf %[[rem]], %[[p]] {{.*}}: f64
! CHECK: %[[res:.*]] = arith.select %[[mustAddP]], %[[remPlusP]], %[[rem]] : f64
- ! CHECK: fir.store %[[res]] to %[[arg0]] : !fir.ref<f64>
+ ! ALL: fir.store %[[res]] to %[[arg0]] : !fir.ref<f64>
r = modulo(a, p)
end subroutine
-! CHECK-LABEL: func @_QPmodulo_testi(
-! CHECK-SAME: %[[arg0:.*]]: !fir.ref<i64>{{.*}}, %[[arg1:.*]]: !fir.ref<i64>{{.*}}, %[[arg2:.*]]: !fir.ref<i64>{{.*}}) {
+! ALL-LABEL: func @_QPmodulo_testi(
+! ALL-SAME: %[[arg0:.*]]: !fir.ref<i64>{{.*}}, %[[arg1:.*]]: !fir.ref<i64>{{.*}}, %[[arg2:.*]]: !fir.ref<i64>{{.*}}) {
subroutine modulo_testi(r, a, p)
integer(8) :: r, a, p
! CHECK-DAG: %[[a:.*]] = fir.load %[[arg1]] : !fir.ref<i64>
diff --git a/flang/unittests/Runtime/Numeric.cpp b/flang/unittests/Runtime/Numeric.cpp
index 43263d1ac42315..3236c76961e631 100644
--- a/flang/unittests/Runtime/Numeric.cpp
+++ b/flang/unittests/Runtime/Numeric.cpp
@@ -76,6 +76,14 @@ TEST(Numeric, Modulo) {
EXPECT_EQ(RTNAME(ModuloReal4)(Real<4>{-8.0}, Real<4>(5.0)), 2.0);
EXPECT_EQ(RTNAME(ModuloReal8)(Real<8>{8.0}, Real<8>(-5.0)), -2.0);
EXPECT_EQ(RTNAME(ModuloReal8)(Real<8>{-8.0}, Real<8>(-5.0)), -3.0);
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal4)(
+ Real<4>{0.5}, std::numeric_limits<Real<4>>::infinity())));
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal4)(
+ Real<4>{-0.5}, std::numeric_limits<Real<4>>::infinity())));
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal8)(
+ Real<8>{-0.5}, std::numeric_limits<Real<8>>::infinity())));
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal8)(
+ Real<8>{0.5}, std::numeric_limits<Real<8>>::infinity())));
}
TEST(Numeric, Nearest) {
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Slava Zakharin (vzakhari) ChangesStraightforward computation of Full diff: https://github.com/llvm/llvm-project/pull/86145.diff 5 Files Affected:
diff --git a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
index eb8f5135ff12e0..88306757c2b06a 100644
--- a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
+++ b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
@@ -5259,9 +5259,12 @@ mlir::Value IntrinsicLibrary::genModulo(mlir::Type resultType,
remainder);
}
+ auto fastMathFlags = builder.getFastMathFlags();
// F128 arith::RemFOp may be lowered to a runtime call that may be unsupported
// on the target, so generate a call to Fortran Runtime's ModuloReal16.
- if (resultType == mlir::FloatType::getF128(builder.getContext()))
+ if (resultType == mlir::FloatType::getF128(builder.getContext()) ||
+ (fastMathFlags & mlir::arith::FastMathFlags::ninf) ==
+ mlir::arith::FastMathFlags::none)
return builder.createConvert(
loc, resultType,
fir::runtime::genModulo(builder, loc, args[0], args[1]));
diff --git a/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp b/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp
index 4dcbd13cb319f5..81d5d21ece7ae1 100644
--- a/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp
+++ b/flang/lib/Optimizer/Builder/Runtime/Numeric.cpp
@@ -118,6 +118,20 @@ struct ForcedMod16 {
}
};
+/// Placeholder for real*10 version of Modulo Intrinsic
+struct ForcedModulo10 {
+ static constexpr const char *name = ExpandAndQuoteKey(RTNAME(ModuloReal10));
+ static constexpr fir::runtime::FuncTypeBuilderFunc getTypeModel() {
+ return [](mlir::MLIRContext *ctx) {
+ auto fltTy = mlir::FloatType::getF80(ctx);
+ auto strTy = fir::ReferenceType::get(mlir::IntegerType::get(ctx, 8));
+ auto intTy = mlir::IntegerType::get(ctx, 8 * sizeof(int));
+ return mlir::FunctionType::get(ctx, {fltTy, fltTy, strTy, intTy},
+ {fltTy});
+ };
+ }
+};
+
/// Placeholder for real*16 version of Modulo Intrinsic
struct ForcedModulo16 {
static constexpr const char *name = ExpandAndQuoteKey(RTNAME(ModuloReal16));
@@ -349,7 +363,13 @@ mlir::Value fir::runtime::genModulo(fir::FirOpBuilder &builder,
// MODULO is lowered into math operations in intrinsics lowering,
// so genModulo() should only be used for F128 data type now.
- if (fltTy.isF128())
+ if (fltTy.isF32())
+ func = fir::runtime::getRuntimeFunc<mkRTKey(ModuloReal4)>(loc, builder);
+ else if (fltTy.isF64())
+ func = fir::runtime::getRuntimeFunc<mkRTKey(ModuloReal8)>(loc, builder);
+ else if (fltTy.isF80())
+ func = fir::runtime::getRuntimeFunc<ForcedModulo10>(loc, builder);
+ else if (fltTy.isF128())
func = fir::runtime::getRuntimeFunc<ForcedModulo16>(loc, builder);
else
fir::intrinsicTypeTODO(builder, fltTy, loc, "MODULO");
diff --git a/flang/runtime/numeric-templates.h b/flang/runtime/numeric-templates.h
index 8ea3daaa57bcf8..3084cd42f48905 100644
--- a/flang/runtime/numeric-templates.h
+++ b/flang/runtime/numeric-templates.h
@@ -242,10 +242,8 @@ inline RT_API_ATTRS T RealMod(
IS_MODULO ? "MODULO with P==0" : "MOD with P==0");
}
if (ISNANTy<T>::compute(a) || ISNANTy<T>::compute(p) ||
- ISINFTy<T>::compute(a)) {
+ ISINFTy<T>::compute(a) || ISINFTy<T>::compute(p)) {
return QNANTy<T>::compute();
- } else if (ISINFTy<T>::compute(p)) {
- return a;
}
T aAbs{ABSTy<T>::compute(a)};
T pAbs{ABSTy<T>::compute(p)};
diff --git a/flang/test/Lower/Intrinsics/modulo.f90 b/flang/test/Lower/Intrinsics/modulo.f90
index 383cb34f83c705..ac18e59033a6b6 100644
--- a/flang/test/Lower/Intrinsics/modulo.f90
+++ b/flang/test/Lower/Intrinsics/modulo.f90
@@ -1,11 +1,13 @@
-! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s
+! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s -check-prefixes=HONORINF,ALL
+! RUN: flang-new -fc1 -menable-no-infs -emit-fir -flang-deprecated-no-hlfir %s -o - | FileCheck %s -check-prefixes=CHECK,ALL
-! CHECK-LABEL: func @_QPmodulo_testr(
-! CHECK-SAME: %[[arg0:.*]]: !fir.ref<f64>{{.*}}, %[[arg1:.*]]: !fir.ref<f64>{{.*}}, %[[arg2:.*]]: !fir.ref<f64>{{.*}}) {
+! ALL-LABEL: func @_QPmodulo_testr(
+! ALL-SAME: %[[arg0:.*]]: !fir.ref<f64>{{.*}}, %[[arg1:.*]]: !fir.ref<f64>{{.*}}, %[[arg2:.*]]: !fir.ref<f64>{{.*}}) {
subroutine modulo_testr(r, a, p)
real(8) :: r, a, p
- ! CHECK-DAG: %[[a:.*]] = fir.load %[[arg1]] : !fir.ref<f64>
- ! CHECK-DAG: %[[p:.*]] = fir.load %[[arg2]] : !fir.ref<f64>
+ ! ALL-DAG: %[[a:.*]] = fir.load %[[arg1]] : !fir.ref<f64>
+ ! ALL-DAG: %[[p:.*]] = fir.load %[[arg2]] : !fir.ref<f64>
+ ! HONORINF: %[[res:.*]] = fir.call @_FortranAModuloReal8(%[[a]], %[[p]]
! CHECK-DAG: %[[rem:.*]] = arith.remf %[[a]], %[[p]] {{.*}}: f64
! CHECK-DAG: %[[zero:.*]] = arith.constant 0.000000e+00 : f64
! CHECK-DAG: %[[remNotZero:.*]] = arith.cmpf une, %[[rem]], %[[zero]] {{.*}} : f64
@@ -15,12 +17,12 @@ subroutine modulo_testr(r, a, p)
! CHECK-DAG: %[[mustAddP:.*]] = arith.andi %[[remNotZero]], %[[signDifferent]] : i1
! CHECK-DAG: %[[remPlusP:.*]] = arith.addf %[[rem]], %[[p]] {{.*}}: f64
! CHECK: %[[res:.*]] = arith.select %[[mustAddP]], %[[remPlusP]], %[[rem]] : f64
- ! CHECK: fir.store %[[res]] to %[[arg0]] : !fir.ref<f64>
+ ! ALL: fir.store %[[res]] to %[[arg0]] : !fir.ref<f64>
r = modulo(a, p)
end subroutine
-! CHECK-LABEL: func @_QPmodulo_testi(
-! CHECK-SAME: %[[arg0:.*]]: !fir.ref<i64>{{.*}}, %[[arg1:.*]]: !fir.ref<i64>{{.*}}, %[[arg2:.*]]: !fir.ref<i64>{{.*}}) {
+! ALL-LABEL: func @_QPmodulo_testi(
+! ALL-SAME: %[[arg0:.*]]: !fir.ref<i64>{{.*}}, %[[arg1:.*]]: !fir.ref<i64>{{.*}}, %[[arg2:.*]]: !fir.ref<i64>{{.*}}) {
subroutine modulo_testi(r, a, p)
integer(8) :: r, a, p
! CHECK-DAG: %[[a:.*]] = fir.load %[[arg1]] : !fir.ref<i64>
diff --git a/flang/unittests/Runtime/Numeric.cpp b/flang/unittests/Runtime/Numeric.cpp
index 43263d1ac42315..3236c76961e631 100644
--- a/flang/unittests/Runtime/Numeric.cpp
+++ b/flang/unittests/Runtime/Numeric.cpp
@@ -76,6 +76,14 @@ TEST(Numeric, Modulo) {
EXPECT_EQ(RTNAME(ModuloReal4)(Real<4>{-8.0}, Real<4>(5.0)), 2.0);
EXPECT_EQ(RTNAME(ModuloReal8)(Real<8>{8.0}, Real<8>(-5.0)), -2.0);
EXPECT_EQ(RTNAME(ModuloReal8)(Real<8>{-8.0}, Real<8>(-5.0)), -3.0);
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal4)(
+ Real<4>{0.5}, std::numeric_limits<Real<4>>::infinity())));
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal4)(
+ Real<4>{-0.5}, std::numeric_limits<Real<4>>::infinity())));
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal8)(
+ Real<8>{-0.5}, std::numeric_limits<Real<8>>::infinity())));
+ EXPECT_TRUE(std::isnan(RTNAME(ModuloReal8)(
+ Real<8>{0.5}, std::numeric_limits<Real<8>>::infinity())));
}
TEST(Numeric, Nearest) {
|
klausler
approved these changes
Mar 21, 2024
jeanPerier
approved these changes
Mar 21, 2024
|
Peter, can you please review additional change in runtime? Testing uncovered some issues with my change and the existing code. |
klausler
approved these changes
Apr 3, 2024
flang/runtime/numeric-templates.h
Outdated
| @@ -242,10 +242,13 @@ inline RT_API_ATTRS T RealMod( | |||
| IS_MODULO ? "MODULO with P==0" : "MOD with P==0"); | |||
| } | |||
| if (ISNANTy<T>::compute(a) || ISNANTy<T>::compute(p) || | |||
| ISINFTy<T>::compute(a)) { | |||
| ISINFTy<T>::compute(a) || | |||
| // Other compilers behave consistently for MOD(x, +/-INF) | |||
There was a problem hiding this comment.
This would be more clear as a distinct else if block.
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.
Straightforward computation of
A − FLOOR (A / P) * Pshouldproduce NaN, when P is infinity. The -menable-no-infs lowering
can still use the relaxed operations sequence.