This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 1008 - allFinite() does not work correctly with fast math enabled
Summary: allFinite() does not work correctly with fast math enabled
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.2
Hardware: x86 - 64-bit Windows
: Normal Feature Request
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.3 1239
  Show dependency treegraph
 
Reported: 2015-05-01 13:55 UTC by alexis.ts
Modified: 2019-12-04 14:35 UTC (History)
3 users (show)



Attachments
MSVC test project (3.69 KB, application/x-zip-compressed)
2015-05-01 14:05 UTC, alexis.ts
no flags Details
self-contained unit test (2.63 KB, text/x-csrc)
2015-10-27 15:50 UTC, Gael Guennebaud
no flags Details

Description alexis.ts 2015-05-01 13:55:55 UTC
As the summary says the allFinite() function does not work properly with fast math enabled when building with MSVC in x64 mode (x86 works for some reason). I did not test if this is an issue on Linux / with other compilers.

A possible solution would be to use std::isfinite() (only available since c++11) or Microsofts finction _finite() (https://msdn.microsoft.com/en-us/library/sb8es7a8%28v=vs.120%29.aspx).

I attached a MSVC project with a test case for this problem (the platform defaults to Win32, so one has to switch to x64 manually).
Comment 1 alexis.ts 2015-05-01 14:05:12 UTC
Created attachment 569 [details]
MSVC test project
Comment 2 Gael Guennebaud 2015-06-15 20:33:33 UTC
Yes, and this is also the case with gcc. Since we now have a coefficient-wise isFinite method in the devel branch, allFinite could be implemented as a shortcut to mat.array().isFinite().all().

In order to keep vectorization, another option would be warp hasNaN into a non inline function such that the compiler cannot do aggressive optimizations:

template<typename T>
EIGEN_DONT_INLINE
bool hasNaN_helper(const T &a, const T &b)
{
  return !((a.array()==b.array()).all());
}

template<typename Derived>
inline bool DenseBase<Derived>::hasNaN() const
{
  typename internal::nested_eval<Derived,2>::type nested(derived());
  return hasNaN_helper(nested,nested);
}
Comment 3 Gael Guennebaud 2015-06-18 11:05:46 UTC
My "dont-inline" trick does not work with gcc. Perhaps we could switch off fast-math using pragmas, e.g.:

#pragma float_control(precise, on, push)
bool ret = !((a.array()==b.array()).all());
#pragma float_control(pop)

or maybe we just do nothing as it seems to be contradictory to enable fast-math while still caring about NaN...
Comment 4 Christoph Hertzberg 2015-06-18 11:50:29 UTC
If we have C++11 or _POSIX_C_SOURCE >= 200112L, we can use isfinite() (and isnan, accordingly). For MSVC we can use _finite(), as suggested in comment 0.
We can still warn that the result of isfinite might be undefined with fast-math enabled.
Comment 5 Gael Guennebaud 2015-06-18 12:14:41 UTC
with fast-mat, c++11 and gcc, std::isnan does not work.
Comment 6 Gael Guennebaud 2015-06-18 12:27:26 UTC
OK, with gcc, if someone want -fast-math but still care about NaN/inf, then he has to compile with:

-ffast-math -fno-finite-math-only

in which case our current implementation is good enough.

With clang, -ffast-math seems to be safe.

So it still remains the question of MSVC. Could someone run the following code with MSVC and /fp:fast /O2

#include <Eigen/Core>
#include <iostream>
using namespace Eigen;
int main() {
  VectorXd m(10);
  m.setRandom();
  m(3) = 0.0/0.0;
  std::cout << "isnan(" << m(3) << ") = " << std::isnan(m(3)) << " == " << numext::isnan(m(3)) << "\n";
  std::cout << "allFinite: " << m.allFinite() << " (0 expected)\n";
  std::cout << "hasNaN:    " << m.hasNaN() << " (1 expected\n";
  m(4) /= 0.0;
  std::cout << "isinf(" << m(4) << ") = " << std::isinf(m(4)) << " == " << numext::isinf(m(4)) << "\n";
  std::cout << "allFinite: " << m.allFinite() << " (0 expected)\n";
  std::cout << "hasNaN:    " << m.hasNaN() << " (1 expected)\n";
  m(3) = 0;
  std::cout << "allFinite: " << m.allFinite() << " (0 expected)\n";
  std::cout << "hasNaN:    " << m.hasNaN() << " (0 expected)\n";
}
Comment 7 Gael Guennebaud 2015-06-18 12:31:04 UTC
Sorry, I've been too fast, with clang, *::isnan, *::isfinite and hasNaN are ok, but allFinite is still over optimized.
Comment 8 alexis.ts 2015-07-28 17:09:51 UTC
I can not build the above code, because numext::isnan() and numext::isinf() are not defined in the 3.2.5 release. Where do they com from?
Comment 9 Gael Guennebaud 2015-07-28 20:55:45 UTC
They are de fined in the devel branch: http://bitbucket.org/eigen/eigen/get/default.zip
Comment 10 alexis.ts 2015-07-29 07:02:27 UTC
This is what I get with VS2013 and VS2015 compiled for 64bit in release and debug mode:

isnan(nan) = 1 == 0
allFinite: 1 (0 expected)
hasNaN:    0 (1 expected
isinf(inf) = 1 == 1
allFinite: 1 (0 expected)
hasNaN:    0 (1 expected)
allFinite: 1 (0 expected)
hasNaN:    0 (0 expected)

I had to change "m(3) = 0.0/0.0;" into "m(3) = std::numeric_limits<double>::quiet_NaN();" since VS2015 treats the former as constexpr and throws an error at compile time.

I looked into numext::isnan() and found out that EIGEN_HAS_CXX11_MATH is set to 0 due to __cplusplus still beeing set to 199711L. VS2015 is still not c++11 feature complete (Expression SFINAE, Attributes and Atomics in signal handlers are missing)...
Comment 11 Gael Guennebaud 2015-10-27 15:48:27 UTC
Some workarounds for clang, gcc 4.6 to 5, and hopefully MSVC as well:

https://bitbucket.org/eigen/eigen/commits/a9b0d314d377/
Summary:     Bug 1008: stabilize isfinite/isinf/isnan/hasNaN/allFinite functions for fast-math mode.

https://bitbucket.org/eigen/eigen/commits/9e973ee8684d/
Summary:     Bug 1008: improve handling of fast-math mode for older gcc versions.

https://bitbucket.org/eigen/eigen/commits/807a624d6977/
Summary:     Bug 1008: add a unit test for fast-math mode and isinf/isnan/isfinite/etc. functions.


To check that everything does work, simply compile and run the new "fastmath" unit test.
Comment 12 Gael Guennebaud 2015-10-27 15:50:47 UTC
Created attachment 618 [details]
self-contained unit test

For the sake of completeness, here is the self-contained .cpp file I used as a basis for the new unit test. Simpler to use for testing different compilers/options.
Comment 13 Gael Guennebaud 2015-10-28 15:55:27 UTC
For the record, here is one more related changeset:

https://bitbucket.org/eigen/eigen/commits/568e2c341544/
Summary:     Enable std::isfinite/nan/inf on MSVC 2013 and newer and clang. Fix isinf for gcc4.4 and older msvc with fast-math.
Comment 14 Gael Guennebaud 2015-12-10 15:22:39 UTC
The current workarounds works with MSVC, GCC, and recent clang, but isinf fails with old clang versions for which std::isinf fails too. So I'd say good enough.
Comment 15 Nobody 2019-12-04 14:35:37 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1008.

Note You need to log in before you can comment on or make changes to this bug.