[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

[Vladislav Dzhidzhoev via Phabricator via cfe-commits]

dzhidzhoev added inline comments.


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:370
+  const auto StorageAlignment = getAlignment(StorageType);
+  if (LayoutSize % StorageAlignment || Layout.getDataSize() % StorageAlignment)
     Packed = true;
----------------
efriedma wrote:
> dzhidzhoev wrote:
> > efriedma wrote:
> > > Should this be `if (Layout.getSize() % StorageAlignment || Layout.getDataSize() % StorageAlignment)`?  The dependency on isNoUniqueAddress is a bit confusing.
> > For base class subobjects, base class DataSize is used to compute the whole object size in RecordLayoutBuilder.cpp, and base subobject LLVM type (with ".base" suffix) is computed in addition to standard layout LLVM type in CGRecordLayoutBuilder.cpp.
> > 
> > Currently, both base subobject and standard layout LLVM types of the same class are supposed to agree on packedness, as stated in bb51300970b7. Thus, if one is packed, both are marked as packed, as done in CGRecordLowering::determinePacked. 
> > 
> > For data members of class/struct type, declared with [[no_unique_address]] attribute, DataSize is used to compute the whole object size in RecordLayoutBuilder.cpp, but standard layout LLVM type is used to represent the field in the whole class's LLVM type.
> > This patch proposes to use the base subobject LLVM type instead of the default LLVM type of a class to lay out a no_unique_address member of this class type.
> > 
> > Nextly, the patch proposes to create an unpadded LLVM type for unions, in addition to the default LLVM type. It is used to lay out the union members having no_unique_address attribute. Existing code for creating base subobject layout of classes are reused for unions, therefore, packedness of potentially-overlapping union LLVM type and of default union LLVM types must agree as well as for classes. 
> That doesn't really get at what I was asking.
> 
> `LayoutSize % StorageAlignment || Layout.getDataSize() % StorageAlignment` is basically equivalent to `isNoUniqueAddress ? Layout.getDataSize() % StorageAlignment != 0 : (Layout.getSize() % StorageAlignment || Layout.getDataSize() % StorageAlignment)`.  From my understanding, you want isNoUniqueAddress=true to agree with isNoUniqueAddress=false on whether the result is packed.  So why are the checks different?
Thanks, sorry for misunderstanding.

It seems that if Layout.getSize() isn't aligned by StorageAlignment, Layout.getDataSize() isn't aligned by it too, thus `Layout.getDataSize() % StorageAlignment` is enough in that condition (at least, it doesn't break any check-clang tests).
I have added an assert on that assumption.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139741/new/

https://reviews.llvm.org/D139741