From 12841928df1bf6ebfb40fc3691a7912262dac66f Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Sun, 13 Apr 2014 11:53:44 +0200 Subject: [PATCH 1/3] BitField: Optimize generated assembly by forcing inlining. --- Source/Core/Common/BitField.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Source/Core/Common/BitField.h b/Source/Core/Common/BitField.h index 40e748229e..e1e5ba6546 100644 --- a/Source/Core/Common/BitField.h +++ b/Source/Core/Common/BitField.h @@ -36,6 +36,8 @@ #include #include +#include "Common.h" + /* * Abstract bitfield class * @@ -113,13 +115,13 @@ public: // so that we can use this within unions BitField() = default; - BitField& operator=(T val) + __forceinline BitField& operator=(T val) { storage = (storage & ~GetMask()) | ((val << position) & GetMask()); return *this; } - operator T() const + __forceinline operator T() const { if (std::numeric_limits::is_signed) { @@ -144,7 +146,7 @@ private: // Unsigned version of StorageType typedef typename std::make_unsigned::type StorageTypeU; - StorageType GetMask() const + __forceinline StorageType GetMask() const { return ((~(StorageTypeU)0) >> (8*sizeof(T) - bits)) << position; } From ccc04944b2847e91fda3f22fae6723394644d75a Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Sun, 13 Apr 2014 11:59:48 +0200 Subject: [PATCH 2/3] BitField: Fix alignment issues. At least one platform (ARM with NEON instructions enabled) generates SIGBUSes if BitField objects aren't aligned properly. --- Source/Core/Common/BitField.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Source/Core/Common/BitField.h b/Source/Core/Common/BitField.h index e1e5ba6546..f7a0a0175c 100644 --- a/Source/Core/Common/BitField.h +++ b/Source/Core/Common/BitField.h @@ -94,13 +94,23 @@ * * Caveats: * + * 1) * BitField provides automatic casting from and to the storage type where * appropriate. However, when using non-typesafe functions like printf, an * explicit cast must be performed on the BitField object to make sure it gets * passed correctly, e.g.: * printf("Value: %d", (s32)some_register.some_signed_fields); * + * 2) + * Not really a caveat, but potentially irritating: This class is used in some + * packed structures that do not guarantee proper alignment. Therefore we have + * to use #pragma pack here not to pack the members of the class, but instead + * to break GCC's assumption that the members of the class are aligned on + * sizeof(StorageType). + * TODO(neobrain): Confirm that this is a proper fix and not just masking + * symptoms. */ +#pragma pack(1) template struct BitField { @@ -160,3 +170,4 @@ private: static_assert(bits <= 8 * sizeof(T), "Invalid number of bits"); static_assert(bits > 0, "Invalid number of bits"); }; +#pragma pack() From 774a394808d6855ccd64574f5e93c1c2a3188f58 Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Sun, 13 Apr 2014 13:47:21 +0200 Subject: [PATCH 3/3] UnitTests: Add a test for BitField behavior on odd structure alignment. --- Source/UnitTests/Common/BitFieldTest.cpp | 42 ++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/Source/UnitTests/Common/BitFieldTest.cpp b/Source/UnitTests/Common/BitFieldTest.cpp index 16efd29d19..800bb84360 100644 --- a/Source/UnitTests/Common/BitFieldTest.cpp +++ b/Source/UnitTests/Common/BitFieldTest.cpp @@ -125,3 +125,45 @@ TEST(BitField, Assignment) EXPECT_EQ(object.regular_field_unsigned, object.at_dword_boundary); } } + +// Test class behavior on oddly aligned structures. +TEST(BitField, Alignment) +{ + #pragma pack(1) + struct OddlyAlignedTestStruct + { + u8 padding; + TestUnion obj; + }; + #pragma pack() + + GC_ALIGNED16(OddlyAlignedTestStruct test_struct); + TestUnion& object = test_struct.obj; + static_assert(alignof(test_struct.obj.signed_1bit) == 1, "Incorrect variable alignment"); + + for (u64 val : table) + { + // Assignments with fixed values + object.full_u64 = val; + EXPECT_EQ(val, object.full_u64); + + object.full_s64 = (s64)val; + EXPECT_EQ((s64)val, object.full_u64); + + object.regular_field_unsigned = val; + EXPECT_EQ(val & 0x7, object.regular_field_unsigned); + + object.at_dword_boundary = val; + EXPECT_EQ(((s64)(val << 60)) >> 60, object.at_dword_boundary); + + object.signed_1bit = val; + EXPECT_EQ((val & 1) ? -1 : 0, object.signed_1bit); + + object.regular_field_signed = val; + EXPECT_EQ(((s64)(object.hex << 61)) >> 61, object.regular_field_signed); + + // Assignment from other BitField + object.at_dword_boundary = object.regular_field_unsigned; + EXPECT_EQ(object.regular_field_unsigned, object.at_dword_boundary); + } +}