[PATCH] D49710: [GlobalMerge] Allow merging globals with arbitrary alignment.

[David Greene via Phabricator via llvm-commits]

greened added inline comments.


================
Comment at: lib/CodeGen/GlobalMerge.cpp:464
       Type *Ty = Globals[j]->getValueType();
+      unsigned Align = Globals[j]->getPointerAlignment(DL);
+      unsigned Padding = alignTo(MergedSize, Align) - MergedSize;
----------------
efriedma wrote:
> greened wrote:
> > The eliminated check below used DL.getPreferredAlignment.  Is this equivalent?  If I understand getPointerAlignment correctly, it is more conservative than DL.getPreferredAlignment.  So it seems the eliminated check was too conservative.  It eliminated things that might not have had a problematic alignment.
> > 
> > Just making sure I'm understanding this correctly.
> For a global with a strong definition, getPointerAlignment() is basically something like `GVar->getAlignment() == 0 ? DL.getPreferredAlignment(GVar) : GVar->getAlignment()`.  That isn't exactly consistent with what the backend would do; getGVAlignmentLog2 seems to do something more like `std::max(DL.getPreferredAlignment(GVar), GVar->getAlignment())`.  This doesn't really matter because getPreferredAlignment itself returns `GVar->getAlignment()` for globals with explicit alignment... unless the explicit alignment of the global is less than the ABI alignment of the global's type, in which case the alignment is forced to the ABI alignment of the type.  But this result is ignored by getGVAlignmentLog2 if the global has a section marking, in favor of the explicit alignment.  (This was introduced in r129428/r129432 for reasons I don't really understand.)
> 
> So it's a mess, but getPointerAlignment() should return the same result except for the weird edge case involving an explicitly under-aligned global.
Ok, and for that weird edge case it would force an alignment greater than the explicit alignment.  That seems fine given the other benefits of this change.   Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D49710