[PATCH] Fix alignment issues in LLVM.

[James Y Knight]

================
Comment at: include/llvm/Support/AlignOf.h:56-57
@@ -47,2 +55,4 @@
+#else
   enum { Alignment =
          static_cast<unsigned int>(sizeof(AlignmentCalcImpl<T>) - sizeof(T)) };
+#endif
----------------
rnk wrote:
> jyknight wrote:
> > majnemer wrote:
> > > Why not just do `enum : unsigned { Alignment = ... ;` ?
> > I hadn't tried until you suggested, but the answer is: because GCC still warns about it.
> A static constexpr int still sometimes requires an out of class definition if you accidentally take the address of the global by passing it to a reference-taking function like std::max, which is a pretty common operation for alignments.
> 
> I'd rather add casts in the assertions or disable gcc's warning. Seem reasonable?
Adding casts everywhere would be pretty annoying. Adding -Wno-enum-compare to the compile flags seems okay, but somewhat unfortunate, as it can usually expose real bugs.

How about just adding a couple lines to the header:
  template <typename T>
  constexpr unsigned AlignOf<T>::Alignment;
?


================
Comment at: lib/IR/Metadata.cpp:393-396
@@ -384,2 +392,6 @@
 void *MDNode::operator new(size_t Size, unsigned NumOps) {
-  void *Ptr = ::operator new(Size + NumOps * sizeof(MDOperand));
+  size_t OpSize = NumOps * sizeof(MDOperand);
+  // uint64_t is the most aligned type we need support (ensured by static_assert
+  // above)
+  size_t AlignOffset = OffsetToAlignment(OpSize, llvm::alignOf<uint64_t>());
+  void *Ptr =
----------------
rnk wrote:
> I feel like a more intuitive way to phrase this computation is to overallocate like so:
>   size_t OpSize = NumOps * sizeof(MDOperand);
>   OpSize = RoundUpToAlignment(OpSize, llvm::alignOf<uint64_t>());
> 
> The placement new operations can use the same negative indexing approach as operator delete, and we never have to consider AlignOffset as a separate quantity.
SGTM, will change it.

================
Comment at: lib/IR/Metadata.cpp:411-412
@@ -395,1 +410,4 @@
+
+  MDOperand *O =
+      reinterpret_cast<MDOperand *>(reinterpret_cast<uintptr_t>(Mem));
   for (MDOperand *E = O - N->NumOperands; O != E; --O)
----------------
rnk wrote:
> What was wrong with the old code here? A strict aliasing warning?
It's fine back the way it was before; I think I must've added some arithmetic in there in a prior revision.

http://reviews.llvm.org/D10271

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/