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 };