| Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
On 11.06.2014 15:39, Christian Seiler wrote:
Hi,
b. Currently, since you detect the C++ version to decide which
header
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
structure.
I don't quite understand your sentence "to not silently include C++11
dependent features which could principally be implemented in C++03
somehow".
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
unsupported/Eigen/CXX11/CoreOrEmulated
- 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
_Z9testEigenv:
movl $-1, %eax
ret
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
(n-m)-Tensor.
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.
Christoph
--
----------------------------------------------
Dipl.-Inf., Dipl.-Math. Christoph Hertzberg
Cartesium 0.049
Universität Bremen
Enrique-Schmidt-Straße 5
28359 Bremen
Tel: +49 (421) 218-64252
----------------------------------------------