llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-x86 Author: Andy Kaylor (andykaylor) <details> <summary>Changes</summary> The LLVM ABI library's RecordType was omitting direct virtual base classes from its base class vector, which was a divergence from the representation in the Clang AST. Clang's AST includes direct virtual bases in both the collection of base classes and the collection of virtual bases. Aligning the handling between the Clang AST and the LLVM ABI RecordType will simplifying porting of ABI classification for future targets. Assisted-by: Cursor / various models --- Full diff: https://github.com/llvm/llvm-project/pull/218546.diff 8 Files Affected: - (modified) clang/lib/CodeGen/QualTypeMapper.cpp (+24-9) - (added) clang/test/CodeGen/X86/x86_64-empty-cxx-member-abi.cpp (+35) - (modified) llvm/include/llvm/ABI/Types.h (+20-6) - (modified) llvm/lib/ABI/Targets/X86.cpp (+6) - (modified) llvm/lib/ABI/Types.cpp (+23-9) - (modified) llvm/unittests/ABI/CMakeLists.txt (+1) - (added) llvm/unittests/ABI/TypesTest.cpp (+138) - (modified) llvm/utils/gn/secondary/llvm/unittests/ABI/BUILD.gn (+4-1) ``````````diff diff --git a/clang/lib/CodeGen/QualTypeMapper.cpp b/clang/lib/CodeGen/QualTypeMapper.cpp index 212a138f9b7b7..7c08fd9968ace 100644 --- a/clang/lib/CodeGen/QualTypeMapper.cpp +++ b/clang/lib/CodeGen/QualTypeMapper.cpp @@ -393,20 +393,28 @@ QualTypeMapper::convertCXXRecordType(const CXXRecordDecl *RD) { if (RD->isPolymorphic()) { const llvm::abi::Type *VtablePointer = createPointerTypeForPointee(ASTCtx.VoidPtrTy); - Fields.emplace_back(VtablePointer, 0); + Fields.emplace_back(VtablePointer, 0, /*IsBitField=*/false, + /*BitFieldWidth=*/0, /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/false, + /*IsVTablePointer=*/true); } for (const auto &Base : RD->bases()) { - if (Base.isVirtual()) - continue; - const RecordType *BaseRT = Base.getType()->castAs<RecordType>(); + const CXXRecordDecl *BaseDecl = BaseRT->getAsCXXRecordDecl(); const llvm::abi::Type *BaseType = convertType(Base.getType()); + // Virtual and non-virtual base offsets live in separate maps in the AST + // record layout. uint64_t BaseOffset = - Layout.getBaseClassOffset(BaseRT->getAsCXXRecordDecl()).getQuantity() * + (Base.isVirtual() ? Layout.getVBaseClassOffset(BaseDecl) + : Layout.getBaseClassOffset(BaseDecl)) + .getQuantity() * 8; - - BaseClasses.emplace_back(BaseType, BaseOffset); + BaseClasses.emplace_back(BaseType, BaseOffset, /*IsBitField=*/false, + /*BitFieldWidth=*/0, /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/false, + /*IsVTablePointer=*/false, + /*IsVirtualBase=*/Base.isVirtual()); } for (const auto &VBase : RD->vbases()) { @@ -417,7 +425,13 @@ QualTypeMapper::convertCXXRecordType(const CXXRecordDecl *RD) { .getQuantity() * 8; - VirtualBaseClasses.emplace_back(VBaseType, VBaseOffset); + VirtualBaseClasses.emplace_back(VBaseType, VBaseOffset, + /*IsBitField=*/false, + /*BitFieldWidth=*/0, + /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/false, + /*IsVTablePointer=*/false, + /*IsVirtualBase=*/true); } computeFieldInfo(RD, Fields, Layout); @@ -568,8 +582,9 @@ void QualTypeMapper::computeFieldInfo( IsUnnamedBitField = FD->isUnnamedBitField(); } + bool HasNoUniqueAddress = FD->hasAttr<NoUniqueAddressAttr>(); Fields.emplace_back(FieldType, OffsetInBits, IsBitField, BitFieldWidth, - IsUnnamedBitField); + IsUnnamedBitField, HasNoUniqueAddress); ++FieldIndex; } } diff --git a/clang/test/CodeGen/X86/x86_64-empty-cxx-member-abi.cpp b/clang/test/CodeGen/X86/x86_64-empty-cxx-member-abi.cpp new file mode 100644 index 0000000000000..7ce36fc620707 --- /dev/null +++ b/clang/test/CodeGen/X86/x86_64-empty-cxx-member-abi.cpp @@ -0,0 +1,35 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -emit-llvm %s -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -emit-llvm -fexperimental-abi-lowering %s -o - | FileCheck %s + +typedef float V4F __attribute__((vector_size(16))); + +struct Empty {}; + +extern "C" { + +union VecAndEmpty { + V4F v; + Empty e; +}; + +void take_vec_and_empty(union VecAndEmpty u); +void call_vec_and_empty(union VecAndEmpty u) { take_vec_and_empty(u); } +// CHECK-DAG: declare void @take_vec_and_empty(<2 x double>) + +union VecAndEmpty ret_vec_and_empty(void); +void call_ret_vec_and_empty(void) { ret_vec_and_empty(); } +// CHECK-DAG: declare <2 x double> @ret_vec_and_empty() + +struct VecNoUniqueAddress { + V4F v; + [[no_unique_address]] Empty e; +}; + +void take_vec_nua(VecNoUniqueAddress s); +void call_vec_nua(VecNoUniqueAddress s) { take_vec_nua(s); } +// CHECK-DAG: declare void @take_vec_nua(<4 x float>) + +VecNoUniqueAddress ret_vec_nua(void); +void call_ret_vec_nua(void) { ret_vec_nua(); } +// CHECK-DAG: declare <4 x float> @ret_vec_nua() +} diff --git a/llvm/include/llvm/ABI/Types.h b/llvm/include/llvm/ABI/Types.h index 9ae5e8ea49c37..2247cc59b3978 100644 --- a/llvm/include/llvm/ABI/Types.h +++ b/llvm/include/llvm/ABI/Types.h @@ -237,13 +237,19 @@ struct FieldInfo { uint64_t BitFieldWidth; bool IsBitField; bool IsUnnamedBitfield; + bool HasNoUniqueAddress; + bool IsVTablePointer; + bool IsVirtualBase; FieldInfo(const Type *FieldType, uint64_t OffsetInBits = 0, bool IsBitField = false, uint64_t BitFieldWidth = 0, - bool IsUnnamedBitField = false) + bool IsUnnamedBitField = false, bool HasNoUniqueAddress = false, + bool IsVTablePointer = false, bool IsVirtualBase = false) : FieldType(FieldType), OffsetInBits(OffsetInBits), BitFieldWidth(BitFieldWidth), IsBitField(IsBitField), - IsUnnamedBitfield(IsUnnamedBitField) {} + IsUnnamedBitfield(IsUnnamedBitField), + HasNoUniqueAddress(HasNoUniqueAddress), + IsVTablePointer(IsVTablePointer), IsVirtualBase(IsVirtualBase) {} LLVM_ABI bool isEmpty() const; }; @@ -304,7 +310,16 @@ class RecordType : public Type { return static_cast<unsigned>(Flags & RecordFlags::IsTransparent) != 0; } ArrayRef<FieldInfo> getFields() const { return Fields; } + + /// Returns the direct base classes, both virtual and non-virtual, mirroring + /// clang::CXXRecordDecl::bases(). A virtual base is marked with + /// FieldInfo::IsVirtualBase, and its offset is only meaningful when this + /// record is the most-derived object. ArrayRef<FieldInfo> getBaseClasses() const { return BaseClasses; } + + /// Returns the virtual base classes, both direct and indirect, mirroring + /// clang::CXXRecordDecl::vbases(). Direct virtual bases therefore appear + /// both here and in getBaseClasses(). ArrayRef<FieldInfo> getVirtualBaseClasses() const { return VirtualBaseClasses; } @@ -409,10 +424,9 @@ class TypeBuilder { FieldInfo *FieldArray = Allocator.Allocate<FieldInfo>(Fields.size()); for (size_t I = 0, E = Fields.size(); I != E; ++I) { - const FieldInfo &Field = Fields[I]; - new (&FieldArray[I]) - FieldInfo(Field.FieldType, 0, Field.IsBitField, Field.BitFieldWidth, - Field.IsUnnamedBitfield); + FieldInfo Field = Fields[I]; + Field.OffsetInBits = 0; + new (&FieldArray[I]) FieldInfo(Field); } ArrayRef<FieldInfo> FieldsRef(FieldArray, Fields.size()); diff --git a/llvm/lib/ABI/Targets/X86.cpp b/llvm/lib/ABI/Targets/X86.cpp index 43e30c5a43a18..db547b577df32 100644 --- a/llvm/lib/ABI/Targets/X86.cpp +++ b/llvm/lib/ABI/Targets/X86.cpp @@ -512,6 +512,9 @@ void X86_64TargetInfo::classify(const Type *T, uint64_t OffsetBase, Class &Lo, // If this is a C++ record, classify the bases first. if (RT->isCXXRecord()) { for (const auto &Base : RT->getBaseClasses()) { + // A class with a virtual base has a non-trivial copy constructor, so + // getRecordArgABI() above returned before we got here. + assert(!Base.IsVirtualBase && "Unexpected base class!"); // Classify this field. // @@ -942,6 +945,9 @@ static bool bitsContainNoUserData(const Type *Ty, unsigned StartBit, if (RT->isCXXRecord()) { for (unsigned I = 0; I < RT->getNumBaseClasses(); ++I) { const FieldInfo &Base = RT->getBaseClasses()[I]; + // This only runs for types being passed in registers, which cannot + // have virtual bases. + assert(!Base.IsVirtualBase && "Unexpected base class!"); if (Base.OffsetInBits >= EndBit) continue; diff --git a/llvm/lib/ABI/Types.cpp b/llvm/lib/ABI/Types.cpp index 78132aa71fa97..2e3ec907cf251 100644 --- a/llvm/lib/ABI/Types.cpp +++ b/llvm/lib/ABI/Types.cpp @@ -13,10 +13,10 @@ using namespace llvm; using namespace llvm::abi; bool RecordType::isEmpty() const { - if (hasFlexibleArrayMember() || isPolymorphic() || - getNumVirtualBaseClasses() != 0) + if (hasFlexibleArrayMember()) return false; + // Direct virtual bases are not included in the base class list. for (const FieldInfo &Base : getBaseClasses()) { const auto *BaseRT = dyn_cast<RecordType>(Base.FieldType); if (!BaseRT || !BaseRT->isEmpty()) @@ -24,6 +24,9 @@ bool RecordType::isEmpty() const { } for (const FieldInfo &FI : getFields()) { + // Don't treat vtable pointers as a source field for emptiness. + if (FI.IsVTablePointer) + continue; if (!FI.isEmpty()) return false; } @@ -39,6 +42,10 @@ RecordType::getElementContainingOffset(unsigned OffsetInBits) const { }; for (const FieldInfo &Base : getBaseClasses()) { + // Direct virtual bases are revisited by the virtual base loop below, which + // also covers the indirect ones. + if (Base.IsVirtualBase) + continue; const auto *BaseRT = dyn_cast<RecordType>(Base.FieldType); if ((!BaseRT || !BaseRT->isEmpty()) && Contains(Base)) return &Base; @@ -63,18 +70,25 @@ RecordType::getElementContainingOffset(unsigned OffsetInBits) const { bool FieldInfo::isEmpty() const { if (IsUnnamedBitfield) return true; - if (IsBitField && BitFieldWidth == 0) - return true; const Type *Ty = FieldType; + bool WasArray = false; while (const auto *AT = dyn_cast<ArrayType>(Ty)) { - if (AT->getNumElements() != 1) - break; + // Constant arrays of zero length always count as empty. + if (AT->getNumElements() == 0) + return true; Ty = AT->getElementType(); + WasArray = true; } - if (const auto *RT = dyn_cast<RecordType>(Ty)) - return RT->isEmpty(); + const auto *RT = dyn_cast<RecordType>(Ty); + if (!RT) + return false; + + // C++ record fields are never empty unless [[no_unique_address]] applies. + // That exception does not apply to arrays of C++ empty records. + if (RT->isCXXRecord() && (WasArray || !HasNoUniqueAddress)) + return false; - return Ty->isZeroSize(); + return RT->isEmpty(); } diff --git a/llvm/unittests/ABI/CMakeLists.txt b/llvm/unittests/ABI/CMakeLists.txt index fe0431524c7c2..65166e676ac01 100644 --- a/llvm/unittests/ABI/CMakeLists.txt +++ b/llvm/unittests/ABI/CMakeLists.txt @@ -6,4 +6,5 @@ set(LLVM_LINK_COMPONENTS add_llvm_unittest(ABITests AArch64TargetInfoTest.cpp + TypesTest.cpp ) diff --git a/llvm/unittests/ABI/TypesTest.cpp b/llvm/unittests/ABI/TypesTest.cpp new file mode 100644 index 0000000000000..ad448882b82b1 --- /dev/null +++ b/llvm/unittests/ABI/TypesTest.cpp @@ -0,0 +1,138 @@ +//===- TypesTest.cpp - ABI type emptiness unit tests ----------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/ABI/Types.h" +#include "llvm/Support/Alignment.h" +#include "llvm/Support/Allocator.h" +#include "llvm/Support/Casting.h" +#include "llvm/Support/TypeSize.h" +#include "gtest/gtest.h" + +using llvm::Align; +using llvm::TypeSize; +using llvm::abi::FieldInfo; +using llvm::abi::RecordFlags; +using llvm::abi::RecordType; +using llvm::abi::StructPacking; +using llvm::abi::TypeBuilder; + +namespace { + +class ABITypesTest : public ::testing::Test { +protected: + llvm::BumpPtrAllocator Alloc; + TypeBuilder TB; + + ABITypesTest() : TB(Alloc) {} + + const RecordType *makeRecord(llvm::ArrayRef<FieldInfo> Fields, + uint64_t SizeBits, RecordFlags Flags, + llvm::ArrayRef<FieldInfo> Bases = {}, + llvm::ArrayRef<FieldInfo> VBases = {}) { + return TB.getRecordType(Fields, TypeSize::getFixed(SizeBits), Align(1), + StructPacking::Default, Bases, VBases, Flags); + } +}; + +TEST_F(ABITypesTest, EmptyCRecord) { + const RecordType *Empty = makeRecord({}, 0, RecordFlags::CanPassInRegisters); + EXPECT_TRUE(Empty->isEmpty()); +} + +TEST_F(ABITypesTest, NestedEmptyCRecordField) { + const RecordType *Empty = makeRecord({}, 8, RecordFlags::CanPassInRegisters); + const RecordType *Nested = + makeRecord({FieldInfo(Empty, 0)}, 8, RecordFlags::CanPassInRegisters); + EXPECT_TRUE(Nested->isEmpty()); +} + +TEST_F(ABITypesTest, CXXNestedEmptyFieldRequiresNoUniqueAddress) { + RecordFlags CXXFlags = static_cast<RecordFlags>( + RecordFlags::CanPassInRegisters | RecordFlags::IsCXXRecord); + const RecordType *Empty = makeRecord({}, 8, CXXFlags); + + const RecordType *WithoutNUA = makeRecord({FieldInfo(Empty, 0)}, 8, CXXFlags); + EXPECT_FALSE(WithoutNUA->isEmpty()); + + FieldInfo NUAField(Empty, 0, /*IsBitField=*/false, /*BitFieldWidth=*/0, + /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/true); + const RecordType *WithNUA = makeRecord({NUAField}, 8, CXXFlags); + EXPECT_TRUE(WithNUA->isEmpty()); +} + +TEST_F(ABITypesTest, ArrayOfEmptyRecords) { + RecordFlags CFlags = RecordFlags::CanPassInRegisters; + RecordFlags CXXFlags = static_cast<RecordFlags>( + RecordFlags::CanPassInRegisters | RecordFlags::IsCXXRecord); + const RecordType *EmptyC = makeRecord({}, 8, CFlags); + const RecordType *EmptyCXX = makeRecord({}, 8, CXXFlags); + const llvm::abi::Type *ArrC = TB.getArrayType(EmptyC, 2, 16); + const llvm::abi::Type *ArrCXX = TB.getArrayType(EmptyCXX, 2, 16); + const llvm::abi::Type *ZeroArrCXX = TB.getArrayType(EmptyCXX, 0, 0); + + EXPECT_TRUE(makeRecord({FieldInfo(ArrC, 0)}, 16, CFlags)->isEmpty()); + EXPECT_FALSE(makeRecord({FieldInfo(ArrCXX, 0)}, 16, CXXFlags)->isEmpty()); + EXPECT_TRUE(makeRecord({FieldInfo(ZeroArrCXX, 0)}, 0, CXXFlags)->isEmpty()); +} + +TEST_F(ABITypesTest, BitfieldsAndFlexibleArrays) { + const llvm::abi::Type *I32 = TB.getIntegerType(32, Align(4), /*Signed=*/true); + FieldInfo Unnamed(I32, 0, /*IsBitField=*/true, /*BitFieldWidth=*/3, + /*IsUnnamedBitField=*/true); + FieldInfo NamedZero(I32, 0, /*IsBitField=*/true, /*BitFieldWidth=*/0); + + EXPECT_TRUE( + makeRecord({Unnamed}, 8, RecordFlags::CanPassInRegisters)->isEmpty()); + EXPECT_FALSE( + makeRecord({NamedZero}, 8, RecordFlags::CanPassInRegisters)->isEmpty()); + EXPECT_FALSE( + makeRecord({}, 0, RecordFlags::HasFlexibleArrayMember)->isEmpty()); +} + +TEST_F(ABITypesTest, DirectVirtualBasesAndVTablePointer) { + RecordFlags CXXFlags = static_cast<RecordFlags>( + RecordFlags::CanPassInRegisters | RecordFlags::IsCXXRecord); + const RecordType *Empty = makeRecord({}, 8, CXXFlags); + const RecordType *IntField = makeRecord( + {FieldInfo(TB.getIntegerType(32, Align(4), /*Signed=*/true), 0)}, 32, + CXXFlags); + const llvm::abi::Type *VPtr = TB.getPointerType(64, Align(8)); + FieldInfo VTable(VPtr, 0, /*IsBitField=*/false, /*BitFieldWidth=*/0, + /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/false, + /*IsVTablePointer=*/true); + // Direct virtual bases appear in both the base-class list (with + // IsVirtualBase) and the virtual-base list, matching CXXRecordDecl::bases() + // and vbases(). + FieldInfo EmptyVBase(Empty, 0, /*IsBitField=*/false, /*BitFieldWidth=*/0, + /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/false, + /*IsVTablePointer=*/false, + /*IsVirtualBase=*/true); + FieldInfo NonEmptyVBase(IntField, 0, /*IsBitField=*/false, + /*BitFieldWidth=*/0, + /*IsUnnamedBitField=*/false, + /*HasNoUniqueAddress=*/false, + /*IsVTablePointer=*/false, + /*IsVirtualBase=*/true); + + EXPECT_TRUE(makeRecord({}, 8, CXXFlags, /*Bases=*/{EmptyVBase}, + /*VBases=*/{EmptyVBase}) + ->isEmpty()); + EXPECT_FALSE(makeRecord({}, 32, CXXFlags, /*Bases=*/{NonEmptyVBase}, + /*VBases=*/{NonEmptyVBase}) + ->isEmpty()); + EXPECT_TRUE(makeRecord({VTable}, 64, + static_cast<RecordFlags>(CXXFlags | + RecordFlags::IsPolymorphic), + {FieldInfo(Empty, 0)}) + ->isEmpty()); +} + +} // namespace diff --git a/llvm/utils/gn/secondary/llvm/unittests/ABI/BUILD.gn b/llvm/utils/gn/secondary/llvm/unittests/ABI/BUILD.gn index a1c51980f0cbe..b355517a9251d 100644 --- a/llvm/utils/gn/secondary/llvm/unittests/ABI/BUILD.gn +++ b/llvm/utils/gn/secondary/llvm/unittests/ABI/BUILD.gn @@ -6,5 +6,8 @@ unittest("ABITests") { "//llvm/lib/IR", "//llvm/lib/Support", ] - sources = [ "AArch64TargetInfoTest.cpp" ] + sources = [ + "AArch64TargetInfoTest.cpp", + "TypesTest.cpp", + ] } `````````` </details> https://github.com/llvm/llvm-project/pull/218546 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
