[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
https://github.com/erichkeane approved this pull request. https://github.com/llvm/llvm-project/pull/201428 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -692,32 +692,44 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd,
cir::RecordType *ty) {
assert(ty->isIncomplete() && "recomputing record layout?");
lowering.lower(/*nonVirtualBaseType=*/false);
- // If we're in C++, compute the base subobject type. For C++ records the base
- // subobject type is always set (matching classic CodeGen). For unions and
- // final classes the base subobject and complete object types are identical
- // (no tail padding can be reused), so baseTy points at the same record as
- // ty. We must still populate baseTy in those cases because callers such as
- // getStorageType(const CXXRecordDecl *) used to lay out potentially-
- // overlapping ([[no_unique_address]]) fields read it unconditionally; a
- // null baseTy would otherwise propagate as a null mlir::Type into the
- // members vector and trip the !empty() assertion in fillOutputFields.
+ // If we're in C++, compute the base subobject type. For C++ records baseTy
+ // defaults to the complete object type and is replaced by a distinct,
+ // smaller record only when the record has tail padding an enclosing
+ // [[no_unique_address]] field can reuse. We must populate baseTy even when
+ // it equals ty because callers such as getStorageType(const CXXRecordDecl *)
+ // read it unconditionally when laying out potentially-overlapping
+ // ([[no_unique_address]]) fields; a null baseTy would otherwise propagate as
+ // a null mlir::Type into the members vector and trip the !empty() assertion
+ // in fillOutputFields.
cir::RecordType baseTy;
if (llvm::isa(rd)) {
baseTy = *ty;
-if (!rd->isUnion() && !rd->hasAttr() &&
-lowering.astRecordLayout.getNonVirtualSize() !=
-lowering.astRecordLayout.getSize()) {
+// A record needs a distinct base-subobject type when its tail padding can
+// be reused by an enclosing [[no_unique_address]] field. For a
+// struct/class that happens when the non-virtual size differs from the
+// complete size; for a union it happens when the data size differs from
+// the complete size (a union has reusable tail padding when one of its
+// members is a [[no_unique_address]] field with tail padding of its own).
+CharUnits baseSize = rd->isUnion()
+ ? lowering.astRecordLayout.getDataSize()
+ : lowering.astRecordLayout.getNonVirtualSize();
+// A union whose data size is zero consists entirely of
+// [[no_unique_address]] empty members. There is no actual data to expose
+// via a base subobject, so leave baseTy pointing at the complete type.
+if (!baseSize.isZero() && baseSize != lowering.astRecordLayout.getSize()) {
adams381 wrote:
You're right — switched the gate to `getNonVirtualSize() != getSize()` with no
`isUnion()` special case. I'd had it backwards: for a union,
`getNonVirtualSize()` already returns the data size when there's reusable tail
padding, but stays at the one-byte minimum when the union is empty. So the
classic gate is correct on both ends — it fires for a padded union (`nvsize=1 <
size=2`) and does not fire for an all-`[[no_unique_address]]`-empty union
(`nvsize == size == 1`, data size 0). The old `getDataSize()`-based gate fired
spuriously on the empty case (`0 != 1`) and built a `getByteArrayType(0)`,
which is what the extra `isZero()` guard was compensating for; both that guard
and the matching `lowerUnion` clause are gone now.
One spot still differs from classic on purpose: the packedness assertion keeps
a union exemption. Classic derives union packedness from the data size for both
layouts, so they always agree; CIR derives it from the layout size (full size
for the complete object, data size for the base), so the two can legitimately
disagree. I left a comment to that effect.
The previous `UnionZeroDataSize` test case actually had a non-empty
`[[no_unique_address]] int` member (data size 4), so it never exercised the
empty-union path — added a genuinely all-empty union case.
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -870,8 +888,20 @@ void CIRRecordLowering::lowerUnion() {
if (layoutSize < getSize(storageType))
storageType = getByteArrayType(layoutSize);
- else
+
+ if (nonVirtualBaseType) {
adams381 wrote:
The divergence is caused by our `appendPaddingBytes` override (lines 198–209):
for unions it routes tail padding into `unionPadding` rather than `fieldTypes`,
because the complete-union path needs padding tracked separately from field
types. Calling `appendPaddingBytes` in the `nonVirtualBaseType=true` path would
silently stash the padding into `unionPadding` instead of `fieldTypes`,
corrupting the base-subobject record layout. So the base path pushes the
padding directly into `fieldTypes` — what `appendPaddingBytes` does in the
non-union case — rather than calling the override, which would redirect it into
`unionPadding`.
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -692,21 +692,28 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd,
cir::RecordType *ty) {
assert(ty->isIncomplete() && "recomputing record layout?");
lowering.lower(/*nonVirtualBaseType=*/false);
- // If we're in C++, compute the base subobject type. For C++ records the base
- // subobject type is always set (matching classic CodeGen). For unions and
- // final classes the base subobject and complete object types are identical
- // (no tail padding can be reused), so baseTy points at the same record as
- // ty. We must still populate baseTy in those cases because callers such as
- // getStorageType(const CXXRecordDecl *) used to lay out potentially-
- // overlapping ([[no_unique_address]]) fields read it unconditionally; a
- // null baseTy would otherwise propagate as a null mlir::Type into the
- // members vector and trip the !empty() assertion in fillOutputFields.
+ // If we're in C++, compute the base subobject type. For C++ records baseTy
+ // defaults to the complete object type and is replaced by a distinct,
+ // smaller record only when the record has tail padding an enclosing
+ // [[no_unique_address]] field can reuse. We must populate baseTy even when
+ // it equals ty because callers such as getStorageType(const CXXRecordDecl *)
+ // read it unconditionally when laying out potentially-overlapping
+ // ([[no_unique_address]]) fields; a null baseTy would otherwise propagate as
+ // a null mlir::Type into the members vector and trip the !empty() assertion
+ // in fillOutputFields.
cir::RecordType baseTy;
if (llvm::isa(rd)) {
baseTy = *ty;
-if (!rd->isUnion() && !rd->hasAttr() &&
-lowering.astRecordLayout.getNonVirtualSize() !=
-lowering.astRecordLayout.getSize()) {
+// A record needs a distinct base-subobject type when its tail padding can
+// be reused by an enclosing [[no_unique_address]] field. For a
+// struct/class that happens when the non-virtual size differs from the
+// complete size; for a union it happens when the data size differs from
+// the complete size (a union has reusable tail padding when one of its
+// members is a [[no_unique_address]] field with tail padding of its own).
+CharUnits baseSize = rd->isUnion()
+ ? lowering.astRecordLayout.getDataSize()
+ : lowering.astRecordLayout.getNonVirtualSize();
+if (baseSize != lowering.astRecordLayout.getSize()) {
adams381 wrote:
Classic CodeGen's gate at `CGRecordLayoutBuilder.cpp:1091` is simply `if
(Builder.Layout.getNonVirtualSize() != Builder.Layout.getSize())` — no
`isUnion()` guard and no `FinalAttr` guard. The old CIR code added both, and
both were wrong. The `FinalAttr` guard was unsound because a `final` class can
still contain `[[no_unique_address]]` members whose base subobject type differs
from their complete type (the `final` keyword prevents *derivation*, not
tail-padding reuse by an enclosing field). Removing it aligns CIR with classic.
The `isUnion()` guard was removed because we now handle unions — using
`getDataSize()` as the base size for unions, exactly as classic does at line
315.
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -72,7 +72,7 @@ struct CIRRecordLowering final {
mlir::Type storageType);
void lower(bool NonVirtualBaseType);
adams381 wrote:
Done.
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -870,8 +888,20 @@ void CIRRecordLowering::lowerUnion() {
if (layoutSize < getSize(storageType))
storageType = getByteArrayType(layoutSize);
- else
+
+ if (nonVirtualBaseType) {
erichkeane wrote:
This is also quite different than classic codegen. Why does it not need to
calculate the padding here?
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -692,21 +692,28 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd,
cir::RecordType *ty) {
assert(ty->isIncomplete() && "recomputing record layout?");
lowering.lower(/*nonVirtualBaseType=*/false);
- // If we're in C++, compute the base subobject type. For C++ records the base
- // subobject type is always set (matching classic CodeGen). For unions and
- // final classes the base subobject and complete object types are identical
- // (no tail padding can be reused), so baseTy points at the same record as
- // ty. We must still populate baseTy in those cases because callers such as
- // getStorageType(const CXXRecordDecl *) used to lay out potentially-
- // overlapping ([[no_unique_address]]) fields read it unconditionally; a
- // null baseTy would otherwise propagate as a null mlir::Type into the
- // members vector and trip the !empty() assertion in fillOutputFields.
+ // If we're in C++, compute the base subobject type. For C++ records baseTy
+ // defaults to the complete object type and is replaced by a distinct,
+ // smaller record only when the record has tail padding an enclosing
+ // [[no_unique_address]] field can reuse. We must populate baseTy even when
+ // it equals ty because callers such as getStorageType(const CXXRecordDecl *)
+ // read it unconditionally when laying out potentially-overlapping
+ // ([[no_unique_address]]) fields; a null baseTy would otherwise propagate as
+ // a null mlir::Type into the members vector and trip the !empty() assertion
+ // in fillOutputFields.
cir::RecordType baseTy;
if (llvm::isa(rd)) {
baseTy = *ty;
-if (!rd->isUnion() && !rd->hasAttr() &&
-lowering.astRecordLayout.getNonVirtualSize() !=
-lowering.astRecordLayout.getSize()) {
+// A record needs a distinct base-subobject type when its tail padding can
+// be reused by an enclosing [[no_unique_address]] field. For a
+// struct/class that happens when the non-virtual size differs from the
+// complete size; for a union it happens when the data size differs from
+// the complete size (a union has reusable tail padding when one of its
+// members is a [[no_unique_address]] field with tail padding of its own).
+CharUnits baseSize = rd->isUnion()
+ ? lowering.astRecordLayout.getDataSize()
+ : lowering.astRecordLayout.getNonVirtualSize();
+if (baseSize != lowering.astRecordLayout.getSize()) {
erichkeane wrote:
I'm pretty suspicious of how different this is from classic codegen. Also, the
FinalAttr check being lost here is also concerning.
What is going on with the differences?
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CIR] Compute union base subobject for tail-padding reuse (PR #201428)
@@ -72,7 +72,7 @@ struct CIRRecordLowering final {
mlir::Type storageType);
void lower(bool NonVirtualBaseType);
erichkeane wrote:
mind as a drive-by fixing the capitalization of `Non` here?
https://github.com/llvm/llvm-project/pull/201428
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
