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

[Chandler Carruth]

  Sorry for the slowness. Once I started I also had to run a bunch of experiments to see why you weren't doing some of the more obvious things (they don't work). Anyways, comments inline.


================
Comment at: include/llvm/Support/Endian.h:56-57
@@ +55,4 @@
+  memcpy(&ret,
+    reinterpret_cast<const detail::AlignmentHelper<value_type,
+                                                   alignment>*>(memory)->buffer,
+    sizeof(value_type));
----------------
This is pretty horrible undefined behavior. =/ I think its even in the space of things that UBSan may eventually catch and reject.

It seems like what you want here is __builtin_assume_aligned(). Why not use that and then when we teach Clang about it, your code will run faster.

The other way I see to make this work is to use an aligned type in the function argument. Something like;

  typedef __attribute__((aligned(16))) char aligned_char;
  ...
  inline value_type read(const aligned_char *memory) {

Seems to work in my rough testing. I like this being in the interface because then we can add a warning to Clang if people call this with a statically known under aligned pointer. However, I'd also be much more OK with you using:

  memcpy(&ret, reinterpret_cast<const aligned_char *>(memory), ...);

Than the alignment helper trick, as this doesn't access the object as you currently do with the '->'.

I realize the existing code was already doing these hacks, but I don't really like hacking on the code so much w/o fixing this.

================
Comment at: include/llvm/Support/AlignOf.h:180
@@ -165,3 +179,3 @@
   // should carry over to the character array in the union.
-  llvm::AlignedCharArrayImpl<AlignOf<AlignerImpl>::Alignment> nonce_member;
+  llvm::AlignedCharArray<AlignOf<AlignerImpl>::Alignment, 1> nonce_member;
 };
----------------
If you're going to remove the separation of concerns between aligning the buffer and sizing the buffer, you should also clean this up to not be implemented with that mechanism.

Essentially, if we go with your patch (which I'm still a bit skeptical of, because I thought I tried something similar and some compiler rejected it), then this should devolve to a pure template wrapper to  convert from the types to a numerical alignment and size.


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