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 1687 - Reconsider usage of EIGEN_FAST_MATH
Summary: Reconsider usage of EIGEN_FAST_MATH
Status: DECISIONNEEDED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - vectorization (show other bugs)
Version: 3.4 (development)
Hardware: All All
: Normal Internal Design
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on: 1259
Blocks: 564 3.x 1693 1699
  Show dependency treegraph
 
Reported: 2019-02-25 10:47 UTC by Christoph Hertzberg
Modified: 2019-07-02 12:55 UTC (History)
5 users (show)



Attachments

Description Christoph Hertzberg 2019-02-25 10:47:38 UTC
Currently setting EIGEN_FAST_MATH to 1 (which it is by default) essentially just means that sin and cos are calculated using SIMD-math.
I assume that meanwhile our implementation is within 1ULP of the accuracy of the `std` implementation, so this flag is nearly irrelevant.
If we want to keep this, I suggest renaming this to something like `EIGEN_USE_SIMD_MATH_FUNCTIONS` and let it enable/disable all other packet-functions (like plog/pexp/...) as well. 

And if we refactor this, I would also suggest removing `HasSin`, `HasCos`, ... from all the packet_traits, and replace the logic by checking if we have `cast_to_int`, `bitshift`, `bittest`, ... (or whatever operations are needed to implement the relevant function).

Also related is Bug 1259. If SVML (or something equivalent) is available we could offer to use that library instead of our own functions.
Comment 1 Christoph Hertzberg 2019-04-11 08:21:12 UTC
I would also like to remove the `unsupported/Eigen/MoreVectorization` module and merge it into the stable part (assuming it works correctly).
Comment 2 Christoph Hertzberg 2019-06-20 17:39:53 UTC
Another small thing which can have a significant impact would be to ignore setting ERRNO when computing roots, i.e., just call `sqrtss` or `sqrtsd` (if SSE is available) instead of `std::sqrt`. See here for a real-world case:
https://stackoverflow.com/questions/56547557/

Actually, it is kind of inconsistent at the moment that `sqrt(ArrayXd(5))` may change ERRNO, but `sqrt(ArrayXd(4))` will not (if vectorization is enabled). If we are really strict with `EIGEN_FAST_MATH==0`, we can't vectorize square-roots (unless we first check that all inputs are valid) -- even though I doubt that many users actually care about ERRNO.
Comment 3 Rasmus Munk Larsen 2019-06-25 17:40:44 UTC
I agree that the meaning of EIGEN_FAST_MATH is not very well defined. Practically speaking, Eigen follows "fast-math" semantics in many cases regardless of the value of EIGEN_FAST_MATH, so I would be happy to to get rid of it. 

As a data point, only a single out of millions of targets depending at Eigen at Google sets EIGEN_FAST_MATH to 0, and as far as I can tell, that is not longer necessary.
Comment 4 Rasmus Munk Larsen 2019-06-25 17:41:12 UTC
Do we know of users who depend on it?
Comment 5 Christoph Hertzberg 2019-06-25 18:12:43 UTC
I don't know of any users depending on it, but knowledge about our user-base is quite sparse. We started thinking about doing a survey a while ago (I'll try to push that).

Is "a single out of millions of targets" meant literally, or is this just a guess?
Comment 6 Rasmus Munk Larsen 2019-06-25 18:30:15 UTC
It was literally one out of several millions. I just got rid of it, so now it is zero. :-)
Comment 7 Gael Guennebaud 2019-07-02 12:55:57 UTC
I agree that EIGEN_FAST_MATH is not relevant anymore. If we want to leverage more control (ignore NaN, Inf, denormals, ERRNO, ULP, etc.), this should be done at the call site IMO.

Regarding sqrt, we already call _mm_sqrt_ss.

Regarding MoreVectorization, I completely forgot about this one, but looking at the code , its integration within Core/ seems to be straightforward (provided it is accurate enough and robust to NaN/Inf/out-of-range).

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