diff --git a/include/ceres/internal/fixed_array.h b/include/ceres/internal/fixed_array.h index 29ef15747..02d3b7140 100644 --- a/include/ceres/internal/fixed_array.h +++ b/include/ceres/internal/fixed_array.h @@ -34,6 +34,7 @@ #include #include +#include "ceres/internal/macros.h" #include "ceres/internal/manual_constructor.h" namespace ceres { @@ -149,7 +150,7 @@ class FixedArray { // Allocate some space, not an array of elements of type T, so that we can // skip calling the T constructors and destructors for space we never use. - ManualConstructor inline_space_[kInlineElements]; + ManualConstructor inline_space_[kInlineElements] CERES_ALIGN_ATTRIBUTE(16); }; // Implementation details follow diff --git a/include/ceres/internal/macros.h b/include/ceres/internal/macros.h index 7dd4e3eb0..0e84e9c22 100644 --- a/include/ceres/internal/macros.h +++ b/include/ceres/internal/macros.h @@ -152,4 +152,19 @@ char (&ArraySizeHelper(const T (&array)[N]))[N]; #define MUST_USE_RESULT #endif +// Platform independent macros to get aligned memory allocations. +// For example +// +// MyFoo my_foo CERES_ALIGN_ATTRIBUTE(16); +// +// Gives us an instance of MyFoo which is aligned at a 16 byte +// boundary. +#if defined(_MSC_VER) +#define CERES_ALIGN_ATTRIBUTE(n) __declspec(align(n)) +#define CERES_ALIGN_OF(T) __alignof(T) +#elif defined(__GNUC__) +#define CERES_ALIGN_ATTRIBUTE(n) __attribute__((aligned(n))) +#define CERES_ALIGN_OF(T) __alignof(T) +#endif + #endif // CERES_PUBLIC_INTERNAL_MACROS_H_ diff --git a/include/ceres/internal/manual_constructor.h b/include/ceres/internal/manual_constructor.h index a1d1f444e..174d35ee2 100644 --- a/include/ceres/internal/manual_constructor.h +++ b/include/ceres/internal/manual_constructor.h @@ -45,60 +45,49 @@ namespace ceres { namespace internal { -// ------- Define ALIGNED_CHAR_ARRAY -------------------------------- +// ------- Define CERES_ALIGNED_CHAR_ARRAY -------------------------------- -#ifndef ALIGNED_CHAR_ARRAY +#ifndef CERES_ALIGNED_CHAR_ARRAY // Because MSVC and older GCCs require that the argument to their alignment // construct to be a literal constant integer, we use a template instantiated // at all the possible powers of two. template struct AlignType { }; template struct AlignType<0, size> { typedef char result[size]; }; -#if defined(_MSC_VER) -#define BASE_PORT_H_ALIGN_ATTRIBUTE(X) __declspec(align(X)) -#define BASE_PORT_H_ALIGN_OF(T) __alignof(T) -#elif defined(__GNUC__) -#define BASE_PORT_H_ALIGN_ATTRIBUTE(X) __attribute__((aligned(X))) -#define BASE_PORT_H_ALIGN_OF(T) __alignof__(T) -#endif -#if defined(BASE_PORT_H_ALIGN_ATTRIBUTE) +#if !defined(CERES_ALIGN_ATTRIBUTE) +#define CERES_ALIGNED_CHAR_ARRAY you_must_define_CERES_ALIGNED_CHAR_ARRAY_for_your_compiler +#else // !defined(CERES_ALIGN_ATTRIBUTE) -#define BASE_PORT_H_ALIGNTYPE_TEMPLATE(X) \ +#define CERES_ALIGN_TYPE_TEMPLATE(X) \ template struct AlignType { \ - typedef BASE_PORT_H_ALIGN_ATTRIBUTE(X) char result[size]; \ + typedef CERES_ALIGN_ATTRIBUTE(X) char result[size]; \ } -BASE_PORT_H_ALIGNTYPE_TEMPLATE(1); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(2); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(4); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(8); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(16); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(32); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(64); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(128); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(256); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(512); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(1024); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(2048); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(4096); -BASE_PORT_H_ALIGNTYPE_TEMPLATE(8192); +CERES_ALIGN_TYPE_TEMPLATE(1); +CERES_ALIGN_TYPE_TEMPLATE(2); +CERES_ALIGN_TYPE_TEMPLATE(4); +CERES_ALIGN_TYPE_TEMPLATE(8); +CERES_ALIGN_TYPE_TEMPLATE(16); +CERES_ALIGN_TYPE_TEMPLATE(32); +CERES_ALIGN_TYPE_TEMPLATE(64); +CERES_ALIGN_TYPE_TEMPLATE(128); +CERES_ALIGN_TYPE_TEMPLATE(256); +CERES_ALIGN_TYPE_TEMPLATE(512); +CERES_ALIGN_TYPE_TEMPLATE(1024); +CERES_ALIGN_TYPE_TEMPLATE(2048); +CERES_ALIGN_TYPE_TEMPLATE(4096); +CERES_ALIGN_TYPE_TEMPLATE(8192); // Any larger and MSVC++ will complain. -#define ALIGNED_CHAR_ARRAY(T, Size) \ - typename AlignType::result +#undef CERES_ALIGN_TYPE_TEMPLATE -#undef BASE_PORT_H_ALIGNTYPE_TEMPLATE -#undef BASE_PORT_H_ALIGN_ATTRIBUTE +#define CERES_ALIGNED_CHAR_ARRAY(T, Size) \ + typename AlignType::result -#else // defined(BASE_PORT_H_ALIGN_ATTRIBUTE) -#define ALIGNED_CHAR_ARRAY you_must_define_ALIGNED_CHAR_ARRAY_for_your_compiler -#endif // defined(BASE_PORT_H_ALIGN_ATTRIBUTE) +#endif // !defined(CERES_ALIGN_ATTRIBUTE) -#undef BASE_PORT_H_ALIGNTYPE_TEMPLATE -#undef BASE_PORT_H_ALIGN_ATTRIBUTE - -#endif // ALIGNED_CHAR_ARRAY +#endif // CERES_ALIGNED_CHAR_ARRAY template class ManualConstructor { @@ -203,10 +192,10 @@ class ManualConstructor { } private: - ALIGNED_CHAR_ARRAY(Type, 1) space_; + CERES_ALIGNED_CHAR_ARRAY(Type, 1) space_; }; -#undef ALIGNED_CHAR_ARRAY +#undef CERES_ALIGNED_CHAR_ARRAY } // namespace internal } // namespace ceres diff --git a/include/ceres/jet.h b/include/ceres/jet.h index e42abb5be..50b5555c5 100644 --- a/include/ceres/jet.h +++ b/include/ceres/jet.h @@ -210,8 +210,20 @@ struct Jet { return *this; } - T a; // The scalar part. + // The infinitesimal part comes before the scalar part to ensure + // alignment. Jets get allocated as an array of type FixedArray + // which can allocate memory on the stack or on the heap depending + // upon the size of the array. We force the memory allocated on the + // stack to be 16 byte boundary aligned, but we also need to ensure + // that the elements of the struct are themselves aligned. Eigen::Matrix v; // The infinitesimal part. + T a; // The scalar part. + + // Needed to make sure that new instances of Jets are properly + // aligned. For more details see + // + // http://eigen.tuxfamily.org/dox/TopicStructHavingEigenMembers.html + EIGEN_MAKE_ALIGNED_OPERATOR_NEW }; // Unary + diff --git a/internal/ceres/autodiff_test.cc b/internal/ceres/autodiff_test.cc index a9fb833f1..d0bf97af6 100644 --- a/internal/ceres/autodiff_test.cc +++ b/internal/ceres/autodiff_test.cc @@ -377,5 +377,30 @@ TEST(AutoDiff, VaryingNumberOfResidualsForOneCostFunctorType) { } } +// This is fragile test that triggers the alignment bug on +// i686-apple-darwin10-llvm-g++-4.2 (GCC) 4.2.1. It is quite possible, +// that other combinations of operating system + compiler will +// re-arrange the operations in this test. +// +// But this is the best (and only) way we know of to trigger this +// problem for now. A more robust solution that guarantees the +// alignment of Eigen types used for automatic differentiation would +// be nice. +TEST(AutoDiff, AlignedAllocationTest) { + // This int is needed to allocate 16 bits on the stack, so that the + // next allocation is not aligned by default. + char y = 0; + + // This is needed to prevent the compiler from optimizing y out of + // this function. + y += 1; + + typedef Jet JetT; + FixedArray x(3); + + // Need this to makes sure that x does not get optimized out. + x[0] = x[0] + JetT(1.0); +} + } // namespace internal } // namespace ceres