Use default alignment if alignof(std::max_align_t) < 16 with C++11.
- As per Andrew Hunter’s comments in the commit which added Jet
alignment when using C++11 here:
https://ceres-solver-review.googlesource.com/#/c/7100, there is wide
lattitude in the standard about what the maximum supported alignment
can be.
- Previously, we were forcing the alignment to 1, if the value of
alignof(std::max_align_t), which we use as a proxy for the maximum
supported alignment on the platform, was < 16.
- An alignment of 1 is not valid for Jets, as it would weaken the
natural alignment of the types within a Jet, which would typically be
4 (32-bit systems) or 8 (64-bit systems), thus resulting in a compiler
error.
- This was reported as issue 235 for Clang 3.8 on i386:
https://github.com/ceres-solver/ceres-solver/issues/235.
Change-Id: Ie39e5499c64f9231f29ebf4392992b5c9ce2e385
diff --git a/include/ceres/internal/port.h b/include/ceres/internal/port.h
index f4dcaee..8660a66 100644
--- a/include/ceres/internal/port.h
+++ b/include/ceres/internal/port.h
@@ -51,9 +51,13 @@
// We allocate some Eigen objects on the stack and other places they
// might not be aligned to 16-byte boundaries. If we have C++11, we
-// can specify their alignment anyway, and thus can safely enable
-// vectorization on those matrices; in C++99, we are out of luck. Figure out
-// what case we're in and write macros that do the right thing.
+// can specify their alignment (which is desirable, as it means we can safely
+// enable vectorisation on matrices). However, the standard gives wide
+// lattitude as to what alignments are legal. It must be the case that
+// alignments up to alignof(std::max_align_t) are valid, but this might be < 16
+// on some platforms, in which case even if using C++11, on these platforms
+// we should not attempt to align to 16-byte boundaries. If using < C++11,
+// we cannot specify the alignment.
#ifdef CERES_USE_CXX11
namespace port_constants {
static constexpr size_t kMaxAlignBytes =
diff --git a/include/ceres/jet.h b/include/ceres/jet.h
index be0ccb0..e6adaa0 100644
--- a/include/ceres/jet.h
+++ b/include/ceres/jet.h
@@ -229,11 +229,13 @@
// The infinitesimal part.
- // We allocate Jets on the stack and other places they
- // might not be aligned to 16-byte boundaries. If we have C++11, we
- // can specify their alignment anyway, and thus can safely enable
- // vectorization on those matrices; in C++99, we are out of luck. Figure out
- // what case we're in and do the right thing.
+ // We allocate Jets on the stack and other places they might not be aligned
+ // to 16-byte boundaries, which would prevent the safe use of vectorisation.
+ // If we have C++11, we can specify the alignment. However, the standard
+ // gives wide lattitude as to what alignments are valid, and it might be that
+ // the maximum supported alignment is < 16, in which case even with C++11, we
+ // cannot specify 16-byte alignment. If using < C++11, we cannot specify
+ // alignment.
#ifndef CERES_USE_CXX11
// fall back to safe version:
Eigen::Matrix<T, N, 1, Eigen::DontAlign> v;
@@ -242,7 +244,9 @@
16 <= ::ceres::port_constants::kMaxAlignBytes;
static constexpr int kAlignHint = kShouldAlignMatrix ?
Eigen::AutoAlign : Eigen::DontAlign;
- static constexpr size_t kAlignment = kShouldAlignMatrix ? 16 : 1;
+ // alignas(0) should always be ignored, in which case this definition of
+ // v should be equivalent to the non-C++11 case.
+ static constexpr size_t kAlignment = kShouldAlignMatrix ? 16 : 0;
alignas(kAlignment) Eigen::Matrix<T, N, 1, kAlignHint> v;
#endif
};
