================
@@ -388,26 +381,18 @@ llvm::TypeSize
RecordType::getTypeSizeInBits(const mlir::DataLayout &dataLayout,
mlir::DataLayoutEntryListRef params) const {
if (isUnion()) {
- mlir::Type largest = getLargestMember(dataLayout);
- if (!largest)
- return llvm::TypeSize::getFixed(0);
- // `getLargestMember` returns the highest-aligned variant (which dictates
- // the union's alignment), not necessarily the largest by size. When the
- // union is `padded` -- i.e., its highest-aligned variant is strictly
- // smaller than its layout size, as happens for any union containing both
- // a small high-alignment scalar and a larger low-alignment array (e.g.,
- // `union { char[16]; size_t; }`) -- `lowerUnion` appended a trailing
- // byte-array member to extend the highest-aligned variant up to the
- // layout size, and `LowerToLLVM` mirrors this by emitting the union as
- // `{largest, padding}`. Include that padding here so `getTypeSize`
- // reports the same size `LowerToLLVM` produces; otherwise a parent
- // record containing the union gets a spurious tail-padding member added
- // by `insertPadding`, making `sizeof(parent)` and array GEPs off by the
- // missing bytes.
- llvm::TypeSize size = dataLayout.getTypeSizeInBits(largest);
- if (mlir::Type tailPad = getPadding())
- size += dataLayout.getTypeSizeInBits(tailPad);
- return size;
+ // The size of a union is the size of the largest member.
+ llvm::ArrayRef<Type> members = getMembers();
+ if (members.empty() || (getPadded() && members.size() == 1))
+ return llvm::TypeSize::getFixed(8);
+
+ mlir::Type largestType =
+ *llvm::max_element(getMembersWithoutPadding(), [&](Type lhs, Type rhs)
{
+ return dataLayout.getTypeSize(lhs) < dataLayout.getTypeSize(rhs);
+ });
+
+ return llvm::alignTo(dataLayout.getTypeSizeInBits(largestType),
+ dataLayout.getTypeABIAlignment(*this));
----------------
erichkeane wrote:
Right, we used to get it right 'by accident', and a 'side effect' of this patch
is returning to the 'right implementation, wrong answer'.
In reality, our size SHOULD be 'largest element aligned-to-alignment-of-union',
which is what is being left here (and why I believe this is the right patch
still). But counting on the 'padding' type size to just 'get the right
alignment' here is just the wrong way to do it, and is both super fragile, and
straight wrong.
While yes, it communicates in an end-run around that the alignment/size is a
certain value, it is just doing so in an incorrect way.
What SHOULD be happening in `RecordType::getTypeABIAlignment` is that the
alignment 'override' of an 'alignas' is an 'early exit' there.
https://github.com/llvm/llvm-project/pull/199292
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
