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

[Chandler Carruth]

  Mostly just nit picks here. Go ahead and commit with these fixes, and other stuff can be addressed in post-commit review... Sorry this was annoying to get to work, C++'s alignment support is well and truly broken.


================
Comment at: include/llvm/Support/AlignOf.h:92
@@ -81,1 +91,3 @@
 #elif defined(__GNUC__) || defined(__IBM_ATTRIBUTES)
+/// \brief Create a type with an aligned char buffer.
+template<std::size_t Alignment, std::size_t Size>
----------------
Redundant with the comment above.

================
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)
----------------
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?

================
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
----------------
Just used AlignedCharArray directly below?

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

================
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));
----------------
80-columns.

================
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);
   }
----------------
This formatting seems odd...

================
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);
----------------
This formatting is odd. Line up with the first parameter? It looks like it'll fit in 80-columns.

================
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));
 }
----------------
Same suggestion as above....


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