Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements

[ Thread Index | Date Index | More Archives ]

On 11.06.2014 15:39, Christian Seiler wrote:

    b. Currently, since you detect the C++ version to decide which
       to include, this breaks people (read: me ;-)) using
       TensorSymmetry/ if they accidentally compile it with C++ <= 03 -
       instead of getting a nice error message, they get lots of
       substitution errors, because the C++11 version of the header is
       not included.

We had a similar discussion about adding C++11 features before. I
think we somehow agreed to not silently include C++11 dependent
features which could principally be implemented in C++03 somehow,
without making it obvious to the user that he depends on C++11. That
was the also the basic purpose of the Eigen/CXX11/... include

I don't quite understand your sentence "to not silently include C++11
dependent features which could principally be implemented in C++03

Ok, lets assume this scenario: ('You' in this case refers to someone using the library)

1. You implement something which includes a CXX11 header and compile it in C++11 mode. 2. You compile the same project (on a different system) with a C++03 compiler and everything happens to work. 3. You add code which "silently" depends on C++11 features (i.e. no C++11 constructs in your code but in the library code) and it works fine using your C++11 compiler 4. You try to compile this on the other system and get strange errors, even though you did not include anything new. You have no idea what went wrong, because your code compiled fine before and you did not introduce C++11 dependencies yourself.

This can be avoided by separating C++11 and C++03 code and producing an error (or at least a warning) the moment you try to include a CXX11 header using a C++03 compiler. That would also mean to move the "TensorCore" stuff out of the CXX11 subdirectory (but keeping the TensorSymmetry inside).

Maybe in this concrete case the simplest solution is to add a C++11 check directly into the CXX11/TensorSymmetry (as you said it will not work with C++03 ever) -- and add a FIXME into CXX11/Tensor to have it moved outside CXX11 eventually.

My proposal for this issue was, to reiterate it in a bit more detail:

  - merge the pull request
  - create unsupported/Eigen/CXX11/Emulated module that includes the
    required emulation stuff
  - create unsupported/Eigen/CXX11/CoreOrEmulated that either includes
    CXX11/Core or CXX11/Emulated depending on compiler version
  - move tensor module to unsupported/Eigen/Tensor (one dir up)
  - change tensor module to include
  - leave TensorSymmetry underneath CXX11 and have it include CXX11/Core
    but not CoreOrEmulated

I agree with the end-result. If you think doing the merge before moving the headers really simplifies things I won't object. But IMO for now we should remove things affecting the Core from the pull request, anyways.

    c. Eigen::array is a bad name decision for emulating std::array,
       since array kind-of-clashes with Eigen::Array - it's going to be
       hell for people using Eigen's traditional capital-A Array
       template if they accidentally have a typo.

Is there actually a practical difference between Eigen::Array<Type,
size, 1> and std::array<Type, size>? I.e., why not allow indexing with
the already existing Eigen::Array and optionally implement a direct
mapping from std::array if that is available? You'd also don't need to
re-implement constructors for fixed sized arrays (up to size 4 at the
moment, but we could easily increase that to 6 or 8, maybe)

In C++11 std::array has a constexpr constructor and in C++14 its
accessors are also constexpr. CXX11Workarounds.h contains some
workarounds that allows C++11 compilers to have constexpr access to
array elements. On the other hand, this does not apply to Eigen::Array,
and I'm not sure how difficult it would be to do this change and be
vary of regressions later.

If it is the case that both constructor and accessors are constexpr,
then the compiler can easily optimize away a lot of things w.r.t. to
the calculation of the linear index from a set of indices and the such.
For example, the internal::array_prod I added is reduced to the correct
number of multiplication assembly instructions when calculating the
total number of elements in a tensor constructor if this is constexpr.

So, from a C++03 version of the Tensor module, where constexpr is not
available, there is indeed no reason not to use Eigen::Array. From a
C++11 standpoint, using Eigen::Array is probably far from optimal.

Not sure if this makes a practical difference. I made some tests using Eigen::Arrays which compiled with -O1 where entirely simplified. E.g.:
  int testEigen() {
    Eigen::Array4i A(0,1,2,3);
    return A(2)-A(3);

resulted basically in

	movl	$-1, %eax

So as long as all information is available to the compiler I doubt the additional constexpr-ness will make a significant difference.
Of course, you will not be able to write things like
  enum {value = testEigen()};

I also think (hope) eventually we can provide prover constexpr support for fixed-sized Eigen objects (see discussion on Bug 820) -- but I don't think this has top-priority at the moment.

4. General observations:

    a. For all evaluators, you assume something like LinearAccessBit,
       which is fine as long as one doesn't implement some kind of
       generalized .block() for tensor and/or Maps with strides.

I agree. One functionality I would expect from Tensors is to get
"Sub-Tensors", e.g. for a (n)-Tensor fix m indexes and get a

OTOH, I don't think this is something that should hold up the merge,
it's just a missing feature (no sub-tensor support yet anyway).

I also agree. I did not interpret your comment as request to delay the merge for that.

Maybe it would be best to leave the Blend functionality out for now and
add it later to all of Eigen? (Or add it first, if it's something that
can be done very soon and then merge this.)

Adding clean blend functionality requires some basic discussion on how to implement boolean packets. On machine level they should be equivalent to what the vector comparison engines result in (i.e. bit-masks). This might not be the best format for storage, however (depending on individual needs, of course).

So if you are in a hurry merging the Tensor pull request, I'd suggest leaving it out for now.


Dipl.-Inf., Dipl.-Math. Christoph Hertzberg
Cartesium 0.049
Universität Bremen
Enrique-Schmidt-Straße 5
28359 Bremen

Tel: +49 (421) 218-64252

Mail converted by MHonArc 2.6.19+