[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
andrew.w.kaylor accepted this revision. andrew.w.kaylor added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760 case options::OPT_fno_honor_nans: HonorNaNs = false;break; case options::OPT_fapprox_func: ApproxFunc = true;break; case options::OPT_fno_approx_func: ApproxFunc = false; break; joerg wrote: > masoud.ataei wrote: > > masoud.ataei wrote: > > > andrew.w.kaylor wrote: > > > > Should this also imply "MathErrno = false"? > > > I don't think setting ApproxFunc to true should imply "MathErrno = > > > false". > > > > > > Let say someone have a math library that compute approximate result for > > > none special input/output but returns NaN, INF and errno correctly > > > otherwise. That is actually can be fairly common, because performance in > > > the none special cases are much more important that the special ones. So > > > returning errno in the special outputs theoretically should not effect > > > the performance on the main path. Therefore, I think compiler should not > > > assume anything about MathErrno value based on ApproxFunc value. > > I am not sure what I was suggesting in my last comment is correct or not. > > Can one of the more experienced reviewers confirm? > > The question is: Should "ApproxFunc=ture" implies "MathErrno=false"? > They seem pretty orthogonal to me. I see the point of your explanation. I'm satisfied that they should remain separate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
masoud.ataei added a comment. A gentle reminder for reviewers. -- This patch will fix the bug reported here: https://bugs.llvm.org/show_bug.cgi?id=52565 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
joerg added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760 case options::OPT_fno_honor_nans: HonorNaNs = false;break; case options::OPT_fapprox_func: ApproxFunc = true;break; case options::OPT_fno_approx_func: ApproxFunc = false; break; masoud.ataei wrote: > masoud.ataei wrote: > > andrew.w.kaylor wrote: > > > Should this also imply "MathErrno = false"? > > I don't think setting ApproxFunc to true should imply "MathErrno = false". > > > > Let say someone have a math library that compute approximate result for > > none special input/output but returns NaN, INF and errno correctly > > otherwise. That is actually can be fairly common, because performance in > > the none special cases are much more important that the special ones. So > > returning errno in the special outputs theoretically should not effect the > > performance on the main path. Therefore, I think compiler should not assume > > anything about MathErrno value based on ApproxFunc value. > I am not sure what I was suggesting in my last comment is correct or not. Can > one of the more experienced reviewers confirm? > The question is: Should "ApproxFunc=ture" implies "MathErrno=false"? They seem pretty orthogonal to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
masoud.ataei added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760 case options::OPT_fno_honor_nans: HonorNaNs = false;break; case options::OPT_fapprox_func: ApproxFunc = true;break; case options::OPT_fno_approx_func: ApproxFunc = false; break; masoud.ataei wrote: > andrew.w.kaylor wrote: > > Should this also imply "MathErrno = false"? > I don't think setting ApproxFunc to true should imply "MathErrno = false". > > Let say someone have a math library that compute approximate result for none > special input/output but returns NaN, INF and errno correctly otherwise. That > is actually can be fairly common, because performance in the none special > cases are much more important that the special ones. So returning errno in > the special outputs theoretically should not effect the performance on the > main path. Therefore, I think compiler should not assume anything about > MathErrno value based on ApproxFunc value. I am not sure what I was suggesting in my last comment is correct or not. Can one of the more experienced reviewers confirm? The question is: Should "ApproxFunc=ture" implies "MathErrno=false"? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
aaron.ballman edited reviewers, added: joerg, rjmccall; removed: aaron.ballman. aaron.ballman added a comment. Removing myself as a reviewer because this is just far enough outside my expertise to feel uncomfortable reviewing it, but added a few trusted folks who have expressed opinions in this area before. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
masoud.ataei added a comment. Gentle reminder for reviewers. -- This PR is ready for review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
masoud.ataei marked 2 inline comments as done. masoud.ataei added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760 case options::OPT_fno_honor_nans: HonorNaNs = false;break; case options::OPT_fapprox_func: ApproxFunc = true;break; case options::OPT_fno_approx_func: ApproxFunc = false; break; andrew.w.kaylor wrote: > Should this also imply "MathErrno = false"? I don't think setting ApproxFunc to true should imply "MathErrno = false". Let say someone have a math library that compute approximate result for none special input/output but returns NaN, INF and errno correctly otherwise. That is actually can be fairly common, because performance in the none special cases are much more important that the special ones. So returning errno in the special outputs theoretically should not effect the performance on the main path. Therefore, I think compiler should not assume anything about MathErrno value based on ApproxFunc value. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2762 case options::OPT_fno_approx_func: ApproxFunc = false; break; case options::OPT_fmath_errno: MathErrno = true; break; case options::OPT_fno_math_errno: MathErrno = false;break; andrew.w.kaylor wrote: > Should this conflict with -fapprox-func? Same as above. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
masoud.ataei updated this revision to Diff 389820. masoud.ataei edited the summary of this revision. masoud.ataei added a comment. Updated the test combining -ffast-math and -fno-approx-func options. @andrew.w.kaylor I hope you don't mind that I put those tests on `clang/test/Driver/fast-math.c` instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 Files: clang/lib/Driver/ToolChains/Clang.cpp clang/test/Driver/fast-math.c clang/test/Preprocessor/aarch64-target-features.c clang/test/Preprocessor/arm-target-features.c Index: clang/test/Preprocessor/arm-target-features.c === --- clang/test/Preprocessor/arm-target-features.c +++ clang/test/Preprocessor/arm-target-features.c @@ -267,7 +267,7 @@ // CHECK-DEFS:#define __ARM_SIZEOF_WCHAR_T 4 // RUN: %clang -target arm-none-linux-gnu -fno-math-errno -fno-signed-zeros\ -// RUN:-fno-trapping-math -fassociative-math -freciprocal-math\ +// RUN:-fno-trapping-math -fassociative-math -freciprocal-math -fapprox-func\ // RUN:-x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FASTMATH %s // RUN: %clang -target arm-none-linux-gnu -ffast-math -x c -E -dM %s -o -\ // RUN:| FileCheck -match-full-lines --check-prefix=CHECK-FASTMATH %s Index: clang/test/Preprocessor/aarch64-target-features.c === --- clang/test/Preprocessor/aarch64-target-features.c +++ clang/test/Preprocessor/aarch64-target-features.c @@ -115,7 +115,7 @@ // CHECK-CRC32: __ARM_FEATURE_CRC32 1 // RUN: %clang -target aarch64-none-linux-gnu -fno-math-errno -fno-signed-zeros\ -// RUN:-fno-trapping-math -fassociative-math -freciprocal-math\ +// RUN:-fno-trapping-math -fassociative-math -freciprocal-math -fapprox-func\ // RUN:-x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s // RUN: %clang -target aarch64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s // RUN: %clang -target arm64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s Index: clang/test/Driver/fast-math.c === --- clang/test/Driver/fast-math.c +++ clang/test/Driver/fast-math.c @@ -70,6 +70,23 @@ // CHECK-NO-NANS-NO-FAST-MATH: "-cc1" // CHECK-NO-NANS-NO-FAST-MATH-NOT: "-menable-no-nans" // +// RUN: %clang -### -ffast-math -fno-approx-func -c %s 2>&1 \ +// RUN: | FileCheck --check-prefix=CHECK-FAST-MATH-NO-APPROX-FUNC %s +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-cc1" +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-menable-no-infs" +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-menable-no-nans" +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-fno-signed-zeros" +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-mreassociate" +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-freciprocal-math" +// CHECK-FAST-MATH-NO-APPROX-FUNC: "-ffp-contract=fast" +// CHECK-FAST-MATH-NO-APPROX-FUNC-NOT: "-ffast-math" +// CHECK-FAST-MATH-NO-APPROX-FUNC-NOT: "-fapprox-func" +// +// RUN: %clang -### -fno-approx-func -ffast-math -c %s 2>&1 \ +// RUN: | FileCheck --check-prefix=CHECK-NO-APPROX-FUNC-FAST-MATH %s +// CHECK-NO-APPROX-FUNC-FAST-MATH: "-cc1" +// CHECK-NO-APPROX-FUNC-FAST-MATH: "-ffast-math" +// // RUN: %clang -### -fapprox-func -c %s 2>&1 \ // RUN: | FileCheck --check-prefix=CHECK-APPROX-FUNC %s // CHECK-APPROX-FUNC: "-cc1" @@ -130,14 +147,14 @@ // RUN: | FileCheck --check-prefix=CHECK-NO-MATH-ERRNO %s // // RUN: %clang -### -fno-math-errno -fassociative-math -freciprocal-math \ -// RUN: -fno-signed-zeros -fno-trapping-math -c %s 2>&1 \ +// RUN: -fno-signed-zeros -fno-trapping-math -fapprox-func -c %s 2>&1 \ // RUN: | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s // CHECK-UNSAFE-MATH: "-cc1" // CHECK-UNSAFE-MATH: "-menable-unsafe-fp-math" // CHECK-UNSAFE-MATH: "-mreassociate" // // RUN: %clang -### -fno-fast-math -fno-math-errno -fassociative-math -freciprocal-math \ -// RUN: -fno-signed-zeros -fno-trapping-math -c %s 2>&1 \ +// RUN: -fno-signed-zeros -fno-trapping-math -fapprox-func -c %s 2>&1 \ // RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH-UNSAFE-MATH %s // CHECK-NO-FAST-MATH-UNSAFE-MATH: "-cc1" // CHECK-NO-FAST-MATH-UNSAFE-MATH: "-menable-unsafe-fp-math" @@ -178,7 +195,7 @@ // RUN: -fno-math-errno -ffp-contract=fast -fno-rounding-math -c %s 2>&1 \ // RUN: | FileCheck --check-prefix=CHECK-FAST-MATH %s // RUN: %clang -### -fno-honor-infinities -fno-honor-nans -fno-math-errno \ -// RUN: -fassociative-math -freciprocal-math -fno-signed-zeros \ +// RUN: -fassociative-math -freciprocal-math -fno-signed-zeros -fapprox-func \ // RUN: -fno-trapping-math -ffp-contract=fast -fno-rounding-math -c %s 2>&1 \ // RUN: | FileCheck
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
andrew.w.kaylor added a comment. Thanks for the patch! This looks mostly good. I have just a few suggestions. Could you add test cases in clang/test/Driver/clang_f_opts.c to verify that the various driver inputs get overridden in the expected way? Without such a test, this behavior is likely to be broken in the future. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760 case options::OPT_fno_honor_nans: HonorNaNs = false;break; case options::OPT_fapprox_func: ApproxFunc = true;break; case options::OPT_fno_approx_func: ApproxFunc = false; break; Should this also imply "MathErrno = false"? Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2762 case options::OPT_fno_approx_func: ApproxFunc = false; break; case options::OPT_fmath_errno: MathErrno = true; break; case options::OPT_fno_math_errno: MathErrno = false;break; Should this conflict with -fapprox-func? Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2880 break; case options::OPT_fno_unsafe_math_optimizations: AssociativeMath = false; I think you need "AppoxFunc = false" here. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2937 // subsequent options conflict then emit warning diagnostic. if (HonorINFs && HonorNaNs && !AssociativeMath && !ReciprocalMath && SignedZeros && TrappingMath && RoundingFPMath && You need a check for "!ApproxFunc" here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114564/new/ https://reviews.llvm.org/D114564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math
masoud.ataei created this revision.
masoud.ataei added reviewers: andrew.w.kaylor, aaron.ballman, erichkeane,
bmahjour.
masoud.ataei added a project: clang.
masoud.ataei requested review of this revision.
Herald added a subscriber: cfe-commits.
Fining the bug number Bug 52565: https://bugs.llvm.org/show_bug.cgi?id=52565
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114564
Files:
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/fast-math.c
clang/test/Preprocessor/aarch64-target-features.c
clang/test/Preprocessor/arm-target-features.c
Index: clang/test/Preprocessor/arm-target-features.c
===
--- clang/test/Preprocessor/arm-target-features.c
+++ clang/test/Preprocessor/arm-target-features.c
@@ -267,7 +267,7 @@
// CHECK-DEFS:#define __ARM_SIZEOF_WCHAR_T 4
// RUN: %clang -target arm-none-linux-gnu -fno-math-errno -fno-signed-zeros\
-// RUN:-fno-trapping-math -fassociative-math -freciprocal-math\
+// RUN:-fno-trapping-math -fassociative-math -freciprocal-math -fapprox-func\
// RUN:-x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FASTMATH %s
// RUN: %clang -target arm-none-linux-gnu -ffast-math -x c -E -dM %s -o -\
// RUN:| FileCheck -match-full-lines --check-prefix=CHECK-FASTMATH %s
Index: clang/test/Preprocessor/aarch64-target-features.c
===
--- clang/test/Preprocessor/aarch64-target-features.c
+++ clang/test/Preprocessor/aarch64-target-features.c
@@ -115,7 +115,7 @@
// CHECK-CRC32: __ARM_FEATURE_CRC32 1
// RUN: %clang -target aarch64-none-linux-gnu -fno-math-errno -fno-signed-zeros\
-// RUN:-fno-trapping-math -fassociative-math -freciprocal-math\
+// RUN:-fno-trapping-math -fassociative-math -freciprocal-math -fapprox-func\
// RUN:-x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
// RUN: %clang -target aarch64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
// RUN: %clang -target arm64-none-linux-gnu -ffast-math -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-FASTMATH %s
Index: clang/test/Driver/fast-math.c
===
--- clang/test/Driver/fast-math.c
+++ clang/test/Driver/fast-math.c
@@ -130,14 +130,14 @@
// RUN: | FileCheck --check-prefix=CHECK-NO-MATH-ERRNO %s
//
// RUN: %clang -### -fno-math-errno -fassociative-math -freciprocal-math \
-// RUN: -fno-signed-zeros -fno-trapping-math -c %s 2>&1 \
+// RUN: -fno-signed-zeros -fno-trapping-math -fapprox-func -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-UNSAFE-MATH %s
// CHECK-UNSAFE-MATH: "-cc1"
// CHECK-UNSAFE-MATH: "-menable-unsafe-fp-math"
// CHECK-UNSAFE-MATH: "-mreassociate"
//
// RUN: %clang -### -fno-fast-math -fno-math-errno -fassociative-math -freciprocal-math \
-// RUN: -fno-signed-zeros -fno-trapping-math -c %s 2>&1 \
+// RUN: -fno-signed-zeros -fno-trapping-math -fapprox-func -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-NO-FAST-MATH-UNSAFE-MATH %s
// CHECK-NO-FAST-MATH-UNSAFE-MATH: "-cc1"
// CHECK-NO-FAST-MATH-UNSAFE-MATH: "-menable-unsafe-fp-math"
@@ -178,7 +178,7 @@
// RUN: -fno-math-errno -ffp-contract=fast -fno-rounding-math -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-FAST-MATH %s
// RUN: %clang -### -fno-honor-infinities -fno-honor-nans -fno-math-errno \
-// RUN: -fassociative-math -freciprocal-math -fno-signed-zeros \
+// RUN: -fassociative-math -freciprocal-math -fno-signed-zeros -fapprox-func \
// RUN: -fno-trapping-math -ffp-contract=fast -fno-rounding-math -c %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-FAST-MATH %s
// CHECK-FAST-MATH: "-cc1"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2873,6 +2873,7 @@
AssociativeMath = true;
ReciprocalMath = true;
SignedZeros = false;
+ ApproxFunc = true;
TrappingMath = false;
FPExceptionBehavior = "";
break;
@@ -2899,6 +2900,7 @@
MathErrno = false;
AssociativeMath = true;
ReciprocalMath = true;
+ ApproxFunc = true;
SignedZeros = false;
TrappingMath = false;
RoundingFPMath = false;
@@ -2914,6 +2916,7 @@
MathErrno = TC.IsMathErrnoDefault();
AssociativeMath = false;
ReciprocalMath = false;
+ ApproxFunc = false;
SignedZeros = true;
// -fno_fast_math restores default denormal and fpcontract handling
DenormalFPMath = DefaultDenormalFPMath;
@@ -2965,7 +2968,7 @@
CmdArgs.push_back("-fmath-errno");
if (!MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros &&
- !TrappingMath)
+ ApproxFunc && !TrappingMath)
