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 1210 - C99 math only functions should be moved to a CXX11 module
Summary: C99 math only functions should be moved to a CXX11 module
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2016-04-25 14:19 UTC by Gael Guennebaud
Modified: 2016-06-02 14:17 UTC (History)
4 users (show)



Attachments

Description Gael Guennebaud 2016-04-25 14:19:11 UTC
In src/Core/SpecialMathFunctions.h several functions seems to be available through C99/C++11 features only. According to our policy, those should be moved to a dedicated CXX11 module:

Eigen/CXX11/Core

CXX11/Tensor would thus include Eigen/CXX11/Core instead of Eigen/Core.



In this module we could also add type aliases as suggested there:

https://bitbucket.org/eigen/eigen/pull-requests/180/alias-template-for-matrix-and-array/diff


OK?
Comment 1 Christoph Hertzberg 2016-04-25 14:38:49 UTC
Yes, makes sense for me. However, we should probably avoid having both
  Eigen/CXX11/Core
and
  unsupported/Eigen/CXX11/Core
in case somebody directly adds unsupported to the include path.

I don't have an overview what parts of the latter are currently in use (and more or less stable) and could actually be moved to the first. E.g., does the tensor module actually work with the emulated CXX11 stuff?
Comment 2 Gael Guennebaud 2016-04-25 15:48:48 UTC
Indeed, unsupported/Eigen/CXX11/Core should be refactored.

At least we have MaxSizeVector which is not C++11 specific at all and that should be moved elsewhere, e.g., in Tensor or Eigen/src/Core/util/ in internal namespace.

Then the parts which can be 100% emulated could also be moved in a non CXX11 module (Eigen/src/Core/util/) just like static_assert in emulated in C++98.

For the rest I don't know...
Comment 3 Benoit Steiner 2016-04-28 18:43:03 UTC
To answer Christoph, the cxx11 emulation works for the tensor code. We use it when compiling with nvcc versions up to 6.5 since c++11 support only starts with 7.0.

I'm not sure that segregating the c++11 features in separate header files is the best strategy. In particular, in the tensor module, I unlock features through #ifdef depending on compiler capability. For example, if the compiler supports c++11, we add support for multithreading and variadic templates. If the compiler supports cuda, we add support for GPUs. And if the cuda compiler is recent enough to support c++11, we unlock all 3 (cuda, multithreading and variadic templates). It would have been difficult to do the same with separate header files, since we end up with so many possible combinations.
Comment 4 Christoph Hertzberg 2016-04-29 16:21:35 UTC
I guess, if the Tensor module is C++03 compatible now, it does not make sense to still have it in a CXX11 folder. However, it would make sense then to make the corresponding unit tests C++03 compatible as well (which they are not) and run them independently of the compile options.

I'll copy the link to the original discussion about C++11 vs C++03 in Eigen here as well:
http://thread.gmane.org/gmane.comp.lib.eigen/4422

Essentially, the problem that may arise is that people unknowingly get dependencies on C++11, even though Eigen claims to be C++03 compatible.
E.g., someone uses the lgamma function on a C++11 system and wants to port to a C++03 system, which would not work, even though he never knowingly added C++11 code.

An alternative would be to provide fallback implementations for C++03. If someone ever wants to implement SSE/AVX packet functions for these, we would need our own implementation anyway -- and the single element function could be simulated by calling the packet function, but using just one argument.
OTOH, I'm not entirely happy with the current arch/*/MathFunctions.h implementations either (lots of duplicate code with still different levels of implementation between platforms; and the CUDA implementation looks completely like boiler-plate code ...)
Comment 5 Christoph Hertzberg 2016-04-29 16:47:20 UTC
Furthermore, there don't seem to be any unit-tests for these special functions at the moment (except for tensor+cuda). And I found at least one implementation error: zeta() is implemented as unary function for Arrays, even though it should be binary. And for zeta(), I actually don't see any actual C++11/C99 dependency.


Regarding the code-duplication of PacketMath.h I actually opened Bug 976 a while ago.
Comment 6 Benoit Steiner 2016-04-29 23:56:50 UTC
I have started to fix some the tensor tests to compile without c++11. I'll continue to fix the tests one at a time over the next few weeks. Once that's done, we can start looking at moving the tensor code outside of the cxx11 directory. But maybe we should release Eigen 3.3 first?
Comment 7 Christoph Hertzberg 2016-04-30 01:58:09 UTC
I agree that moving the Tensor module out of CXX11 is not urgent (and should not block 3.3).

But the question of including C++11 functions into Eigen/Core should be decided. For best compatibility, I'm tending towards copying the corresponding functions into our source tree, e.g., from here:
http://sourceware.org/git/?p=glibc.git;a=tree;f=sysdeps/ieee754
and activate them, if no C99/CXX11 is available (and perhaps with a NON_MPL-guard? Though at the moment we don't do that for our packet functions, either).


This would still left open the question about type aliases (as in the pull-request).
Comment 8 Gael Guennebaud 2016-05-03 19:39:45 UTC
yes, I agree that the best is to import custom implementation of those functions.
Comment 9 Gael Guennebaud 2016-05-18 16:42:14 UTC
hm, I started to import some code for lgamma from either cephes or glibc (in both cases the licence is OK with MPL) but both are rather messy with endianness issues and the likes... I'm not sure anymore that's a good idea in term of effort and maintenance.

Adding a Eigen/CXX11/Math (or whatever name) new module seems to be overkill too. Our discussion about the inclusion of C++11 features is two years old already, and I've now a quite different opinion: I would rather enable all these small features silently and rather request the user to define EIGEN_CXX03_ONLY (or EIGEN_CXX11_ONLY to disable CXX14, etc.) if that's a matter for him, just like we have a EIGEN_MPL2_ONLY option.
Comment 10 Gael Guennebaud 2016-05-18 16:45:59 UTC
Of course, if we go this way, we should clearly add a small "C++11" flag in to doc of each respective function/class, just like the documentations of the STL we can find online. Another argument is that the same hypothetical user could also silently use c++11 features from the STL, so I don't see anymore why we should behave so strictly.
Comment 11 Christoph Hertzberg 2016-05-19 11:25:37 UTC
Good point. By now, most compilers should have pretty stable C++11 support.

About the macro name: Maybe just a single EIGEN_MAX_CXX_LEVEL which defaults to the level of available CXX support, unless it is provided by the user (and can be 2003,2011,2014,... or just 03,11,14,...)?
Comment 12 Gael Guennebaud 2016-05-19 12:09:03 UTC
yes, this sounds better and simpler to use on our side.
Comment 13 Gael Guennebaud 2016-06-02 14:17:11 UTC
Done. The macro name is EIGEN_MAX_CPP_VER.

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