New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
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: 2016-07-25 13:30 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.

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