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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
