| Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
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".
When I first proposed the Tensor module last year, we came to the
conclusion that it would be really tedious to implement tensors in a
way that's compatible with C++03. Therefore, we decided to allow the
Tensor module to require C++11 and - as you said - wanted to make that
requirement clear to users by including Eigen/CXX11/ in the patch.
Now the situation has somewhat changed: The current merge request shows
us that it is indeed possible without too much effort to implement a
(albeit slightly reduced) version of tensor support in C++03 - just
with a maximum number of possible indices.
The problem with the current merge request my comment you quoted was
refering to was the following change to unsupported/Eigen/CXX11/Core:
+#if __cplusplus <= 199711L
+#include "src/Core/util/EmulateCXX11Meta.h"
+#else
#include "src/Core/util/CXX11Workarounds.h"
#include "src/Core/util/CXX11Meta.h"
+#endif
Note that CXX11Workarounds.h contains an error message if you don't
compile the stuff with a C++11 compiler. The problem with this bit of
code here is that for the Tensor module, which now works without C++11
support, this is not a problem, but for the TensorSymmetry module,
where I can tell you right now that it will never work with C++03, this
is a problem.
However, I don't think this change is something that should delay a
merge, because currently you still have to include
unsupported/CXX11/Tensor, so there is actually the oposite case:
something claiming to need C++11 doesn't actually need it, so it
doesn't harm users that just include stuff without thinking it
needs C++11.
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
This has the consequence that:
- Tensor module can now be used without C++11 support
#include <unsupported/Eigen/Tensor>
- Tensor module will automatically load advanced C++11 version if the
compiler supports it, but will fall back to the C++03 version if it
doesn't. Differences between both versions will be:
* C++03: maximum number of indices, C++11: unlimited
as I said before, we should choose a sensible limit, I think
that Fortran's seven (7) is a good idea (current limit for
Benoit's code is five (5))
* otherwise, I don't see anything in the feature set that's
specific to C++11 that doesn't require the user to actively
use C++11 themselves (such as initializer list constructors
etc.)
- TensorSymmetry module still needs C++11
#include <unsupported/Eigen/CXX11/TensorSymmetry>
But this is definitely something that can be done after the pull
request has been merged.
So the only case somebody uses something that's C++11, it's going to be
more than MAX_INDICES incides in a tensor. If we don't make MAX_INDICES
too small, I don't think this is going to be a huge issue.
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.
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).
b. The HasBlend / pblend() stuff is probably also something that
could benefit the rest of Eigen, right? OTOH, the for loop
over
the packet size seems potentially inefficient to me (at least
on
CPUs) - can't you just do a packet() on m_condImpl and store
that
in the selector?
Blend functionality will be required for the Core to efficiently
support vector comparisons and most importantly to have an efficient
Array<bool,...>::select(exprTrue, exprFalse) expression.
For that purpose I doubt that struct {bool select[N];}; allows for
efficient implementations.
Here http://eigen.tuxfamily.org/bz/show_bug.cgi?id=97 the idea of
introducing an EigenBool<type> packet was proposed.
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.)
After those two things I think the next step would be to merge the
rest
of your stuff as-is for the moment being.
I think we also should find an agreement regarding C++11-emulation
first. I don't think including CXX11 headers without proper C++11
support shall be possible.
See my clarifying comment above.
Best regards,
Christian