Disable finite-math-only option with ENABLE_FAST_MATH=1 case to handle NaN and Inf checks correctly#23881
Merged
asmorkalov merged 1 commit intoopencv:4.xfrom Jul 7, 2023
Merged
Conversation
Merged
6 tasks
Contributor
opencv-alalek
left a comment
There was a problem hiding this comment.
I propose to postpone this change to the next release.
-fast-mathmode is disabled by default for reasons. Users should know that they do before enabling that.- low priority fix
| || defined(__PPC64__) \ | ||
| ) | ||
| ) \ | ||
| && !defined(__FAST_MATH__) |
Contributor
There was a problem hiding this comment.
OPENCV_USE_FASTMATH_BUILTINS
It doesn't make sense to disable all builtins for fast-math (just slow down anything).
Related problem is related to cvIsNan() only (probably no problem at all with cvIsNaN() as there is still no test).
Also this check doesn't handle other cases like:
#elif defined __PPC64__ && defined _ARCH_PWR9 && defined(scalar_test_data_class)
#define CV_INLINE_ISNAN_DBL(value) return scalar_test_data_class(value, 0x40);
#define CV_INLINE_ISNAN_FLT(value) CV_INLINE_ISNAN_DBL(value)
#endif
At first we need to start from tests.
CV_INLINE int cvIsNaN( double value )
{
-#if defined CV_INLINE_ISNAN_DBL
+#if defined(CV_INLINE_ISNAN_DBL) && !defined(__FAST_MATH__)
CV_INLINE_ISNAN_DBL(value);
#else
Cv64suf ieee754;
CV_INLINE int cvIsNaN( float value )
{
-#if defined CV_INLINE_ISNAN_FLT
+#if defined(CV_INLINE_ISNAN_FLT) && !defined(__FAST_MATH__)
CV_INLINE_ISNAN_FLT(value);
#else
Cv32suf ieee754;
Some simple test (to avoid reversing of modified implementations):
TEST(Core_FastMath, cvIsNaN)
{
float nf = std::numeric_limits<float>::quiet_NaN();
EXPECT_TRUE(cvIsNaN(nf));
float nd = std::numeric_limits<double>::quiet_NaN();
EXPECT_TRUE(cvIsNaN(nd));
}
BTW, existed isinf() test fails too. Perhaps we need to add similar checks here too.
Contributor
There was a problem hiding this comment.
Anyway own integer-based check is really slow, we should try to use CPU instructions for that.
Looks like compiler optimize NAN checks to false constant in fast-math mode.
Some investigations including -fno-finite-math-only:
07e04a5 to
934dd96
Compare
Contributor
Author
|
@opencv-alalek Could you take a look on the last version? |
Contributor
|
@asmorkalov Please cleanup unrelated commits. |
…e NaN and Inf checks correctly.
934dd96 to
2d92f42
Compare
asmorkalov
pushed a commit
to opencv/opencv_contrib
that referenced
this pull request
Sep 6, 2024
Fix(wechat_code): Modify isnan for compatibility with -ffast_math. #3490 fix #3150 Merge with opencv/opencv#23881 Reference: https://stackoverflow.com/questions/7263404/mingw32-stdisnan-with-ffast-math ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Relates to opencv/opencv_contrib#3150
-fast-mathoption includes -ffinite-math-only option. Compiler expects that floats/doubles could not be NaN/Inf and does optimization accordingly. In particular __builtin_isnan and related built-in returns invalid result or optimized out by compiler.Related discussion in GCC issue tracker: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949
Stackoverflow question: https://stackoverflow.com/questions/69463347/why-does-gcc-ffast-math-disable-the-correct-result-of-isnan-and-isinf
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.