[llvm-commits] [PATCH] [Support][Endian] Add support for specifying the alignment and native unaligned types.

[Michael Spencer]

  Committed as r171406.


================
Comment at: include/llvm/Support/Compiler.h:243
@@ +242,3 @@
+/// \brief Returns a pointer with an assumed alignment.
+#if defined(__GNUC__) && !defined(__clang__)
+# define LLVM_ASSUME_ALIGNED(p, a) __builtin_assume_aligned(p, a)
----------------
Chandler Carruth wrote:
> FIXME to enable this under Clang when we grow support for it?
> 
> Also, do we need to test for a particular version of GCC much like we do for __builtin_trap?
We probably do, but I could not find which version of gcc added this.

================
Comment at: include/llvm/Support/Endian.h:30-34
@@ -27,21 +29,7 @@
 namespace detail {
-
-template<typename value_type, alignment align>
-struct alignment_access_helper;
-
-template<typename value_type>
-struct alignment_access_helper<value_type, aligned>
-{
-  value_type val;
-};
-
-// Provides unaligned loads and stores.
-#pragma pack(push)
-#pragma pack(1)
-template<typename value_type>
-struct alignment_access_helper<value_type, unaligned>
-{
-  value_type val;
+/// \brief ::value is either alignment, or alignof(T) if alignment is 0.
+template<class T, int alignment>
+struct Align {
+  enum {value = alignment == 0 ? AlignOf<T>::Alignment : alignment};
 };
 
----------------
Chandler Carruth wrote:
> Personally, I would inline this enum into the struct below that actually needs it... It doesn't seem terribly generic, and the name 'Align' is pretty vague.
The read and write functions also need it. Renamed.

================
Comment at: include/llvm/Support/Endian.h:36-38
@@ -47,2 +35,5 @@
 
+template<class T, std::size_t alignment>
+struct AlignmentHelper
+  : AlignedCharArray<Align<T, alignment>::value, sizeof(T)> {};
 } // end namespace detail
----------------
Chandler Carruth wrote:
> Just used AlignedCharArray directly below?
Done.

================
Comment at: include/llvm/Support/Endian.h:55-57
@@ +54,5 @@
+
+  memcpy(&ret,
+    LLVM_ASSUME_ALIGNED(memory, (detail::Align<value_type, alignment>::value)),
+    sizeof(value_type));
+  return byte_swap<value_type, endian>(ret);
----------------
Chandler Carruth wrote:
> This formatting is odd. Line up with the first parameter? It looks like it'll fit in 80-columns.
Done.

================
Comment at: include/llvm/Support/Endian.h:66-69
@@ -86,1 +65,6 @@
+  value = byte_swap<value_type, endian>(value);
+  memcpy(
+    LLVM_ASSUME_ALIGNED(memory, (detail::Align<value_type, alignment>::value)),
+    &value,
+    sizeof(value_type));
 }
----------------
Chandler Carruth wrote:
> Same suggestion as above....
Done.

================
Comment at: include/llvm/Support/Endian.h:79-80
@@ -111,6 +78,4 @@
   operator value_type() const {
-    return endian::read_be<value_type, unaligned>(Value);
-  }
-  void operator=(value_type newValue) {
-    endian::write_be<value_type, unaligned>((void *)&Value, newValue);
+    return endian::read<value_type, endian, alignment>
+                       ((const void*)Value.buffer);
   }
----------------
Chandler Carruth wrote:
> This formatting seems odd...
Fixed.

================
Comment at: utils/yaml2obj/yaml2obj.cpp:793
@@ -792,3 +792,3 @@
   char Buffer[sizeof(BLE.Value)];
-  support::endian::write_le<value_type, support::unaligned>(Buffer, BLE.Value);
+  support::endian::write<value_type, support::little, support::unaligned>(Buffer, BLE.Value);
   OS.write(Buffer, sizeof(BLE.Value));
----------------
Chandler Carruth wrote:
> 80-columns.
Fixed.


http://llvm-reviews.chandlerc.com/D200