llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Adam Smith (adams381) <details> <summary>Changes</summary> A struct's `padded` bool only says that padding exists somewhere in the record. It cannot say which member, and it cannot tell compiler-inserted padding from storage the source declared that holds no ABI data, such as an unnamed bit-field unit. Those two need to differ, because padding is reusable tail padding and declared storage is not, so they give different data sizes. Give each member a mark instead: unmarked for source data, `pad`, or `empty`. A record is then empty for the ABI when no member holds data, which `allMembersNonData` reads off the type. This is the first of three PRs, and nothing populates the marks yet, so `padded` stays for now. Retiring it before CIRGen fills the marks in would make every struct claim it has no padding, and the x86_64 classifier would start counting padding arrays as data with no diagnostic. The CIRGen PR comes next, then the bool removal PR. Assisted-by: Cursor / claude-opus-5 --- Patch is 49.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/215174.diff 8 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIRTypes.h (+16-1) - (modified) clang/include/clang/CIR/Dialect/IR/CIRTypes.td (+72-15) - (modified) clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h (+66-31) - (modified) clang/lib/CIR/Dialect/IR/CIRTypes.cpp (+148-30) - (added) clang/test/CIR/IR/invalid-record-member-kinds.cir (+41) - (modified) clang/test/CIR/IR/struct.cir (+57-2) - (modified) clang/unittests/CIR/CMakeLists.txt (+1) - (added) clang/unittests/CIR/RecordMemberKindTest.cpp (+169) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.h b/clang/include/clang/CIR/Dialect/IR/CIRTypes.h index f72d10d236612..f93e9ad24b349 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.h +++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.h @@ -122,6 +122,7 @@ class RecordType : public mlir::Type { bool isComplete() const { return !isIncomplete(); } bool getPacked() const; bool getPadded() const; + llvm::ArrayRef<RecordMemberKind> getMemberKinds() const; bool isClass() const; bool isStruct() const; @@ -133,7 +134,8 @@ class RecordType : public mlir::Type { std::string getPrefixedName() const; void complete(llvm::ArrayRef<mlir::Type> members, bool packed, bool padded, - mlir::Type padding = {}); + mlir::Type padding = {}, + llvm::ArrayRef<RecordMemberKind> memberKinds = {}); uint64_t getElementOffset(const mlir::DataLayout &dataLayout, unsigned idx) const; bool isLayoutIdentical(const RecordType &other); @@ -143,6 +145,19 @@ class RecordType : public mlir::Type { void removeABIConversionNamePrefix(); }; +/// Drop a member-kind list that marks nothing, so that a record whose members +/// all hold data has exactly one spelling. Two storage keys that print +/// identically would otherwise give two unequal types no reader could tell +/// apart. +llvm::ArrayRef<RecordMemberKind> +normalizeRecordMemberKinds(llvm::ArrayRef<RecordMemberKind> memberKinds); + +/// Whether no member of \p recTy holds data, which makes the record empty for +/// the ABI. Vacuously true for a complete record with no members, and false +/// for an incomplete one, whose members are not known yet. A union's +/// tail-padding slot is not a member and does not count. +bool allMembersNonData(RecordType recTy); + } // namespace cir #endif // CLANG_CIR_DIALECT_IR_CIRTYPES_H diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td index 29afaa6d41f4b..365b96cd8e86f 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td +++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td @@ -621,6 +621,37 @@ def CIR_VoidType : CIR_Type<"Void", "void"> { }]; } +//===----------------------------------------------------------------------===// +// RecordMemberKind +// +// What a record member holds, for members that do not hold source data. +//===----------------------------------------------------------------------===// + +def CIR_RecordMemberKind : CIR_I32EnumAttr< + "RecordMemberKind", "what a record member holds", [ + I32EnumAttrCase<"Data", 0, "data">, + I32EnumAttrCase<"Pad", 1, "pad">, + I32EnumAttrCase<"Empty", 2, "empty"> +]> { + let description = [{ + Distinguishes a record member that holds source data from one that does + not. `pad` is storage the compiler inserted to place a later member at its + required offset, and is reusable tail padding when it trails the record. + `empty` is storage the source declared that carries no data for argument + passing: an unnamed bit-field unit, or a field of a record that is empty for + the ABI. Everything else, including a vtable pointer, a base subobject, and + a bit-field unit with a named occupant, is `data`. + + A record is empty for the ABI when no member is `data`, which is vacuously + true for a record with no members. The distinction between `pad` and + `empty` is load-bearing beyond that: only `pad` is reusable, so a record + whose trailing member is an unnamed bit-field unit keeps that unit in its + data size. + }]; + + let genSpecializedAttr = 0; +} + //===----------------------------------------------------------------------===// // StructType // @@ -656,6 +687,11 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ plain struct declarations. Both are semantically identical; the keyword preserves the original source spelling. + A member may carry a `pad` or `empty` mark, described by + `CIR_RecordMemberKind`, saying that it holds no source data. An unmarked + member holds data, and a record whose members are all unmarked carries no + mark list at all. + Examples: ``` @@ -665,6 +701,8 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ !anonymous = !cir.struct<{!u8i}> !rec_packed = !cir.struct<"p1" packed {!u8i, !u8i}> !rec_padded = !cir.struct<"p2" padded {!u8i, !u8i}> + !rec_pad = !cir.struct<"p3" {!u8i, pad !cir.array<!u8i x 3>}> + !rec_empty = !cir.struct<"e" {empty !u8i}> !recursive = !cir.struct<"Node" {!cir.ptr<!cir.struct<"Node">>}> ``` }]; @@ -675,6 +713,7 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ "bool":$incomplete, "bool":$packed, "bool":$padded, + OptionalArrayRefParameter<"cir::RecordMemberKind">:$member_kinds, "bool":$is_class ); @@ -692,10 +731,11 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ "mlir::StringAttr":$name, "bool":$packed, "bool":$padded, - "bool":$is_class + "bool":$is_class, + CArg<"llvm::ArrayRef<cir::RecordMemberKind>", "{}">:$member_kinds ), [{ return $_get($_ctxt, members, name, /*incomplete=*/false, packed, padded, - is_class); + cir::normalizeRecordMemberKinds(member_kinds), is_class); }]>, // Create an identified and incomplete struct/class type. @@ -704,8 +744,9 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ "bool":$is_class ), [{ return $_get($_ctxt, /*members=*/llvm::ArrayRef<mlir::Type>{}, name, - /*incomplete=*/true, /*packed=*/false, - /*padded=*/false, is_class); + /*incomplete=*/true, /*packed=*/false, /*padded=*/false, + /*member_kinds=*/llvm::ArrayRef<cir::RecordMemberKind>{}, + is_class); }]>, // Create an anonymous struct/class type (always complete). @@ -713,10 +754,12 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ "llvm::ArrayRef<mlir::Type>":$members, "bool":$packed, "bool":$padded, - "bool":$is_class + "bool":$is_class, + CArg<"llvm::ArrayRef<cir::RecordMemberKind>", "{}">:$member_kinds ), [{ return $_get($_ctxt, members, mlir::StringAttr{}, /*incomplete=*/false, - packed, padded, is_class); + packed, padded, + cir::normalizeRecordMemberKinds(member_kinds), is_class); }]> ]; @@ -740,11 +783,14 @@ def CIR_StructType : CIR_Type<"Struct", "struct", [ } void complete(llvm::ArrayRef<mlir::Type> members, bool packed, - bool isPadded); + bool isPadded, + llvm::ArrayRef<cir::RecordMemberKind> memberKinds = {}); uint64_t getElementOffset(const mlir::DataLayout &dataLayout, unsigned idx) const; + /// Marks are provenance rather than layout, so two records that differ + /// only in how their members were produced are layout-identical. bool isLayoutIdentical(const StructType &other); // Checks the name of this record to check if it is a 'after' (or during) @@ -800,7 +846,9 @@ def CIR_UnionType : CIR_Type<"Union", "union", [ - Anonymous: no name and a known body. Padded unions carry an explicit tail-padding type to ensure the LLVM struct - that models the union has the correct byte size. + that models the union has the correct byte size. That slot is separate + from the per-member marks described by `CIR_RecordMemberKind`, which say + what each variant holds. The parser rejects a mark on that slot. Examples: @@ -809,6 +857,7 @@ def CIR_UnionType : CIR_Type<"Union", "union", [ !u_incomplete = !cir.union<"U" incomplete> !u_anonymous = !cir.union<{!s32i, !u8i}> !u_padded = !cir.union<"U" {!s32i, !u8i}, padding = {!u8i}> + !u_empty = !cir.union<"U" {empty !u8i}> ``` }]; @@ -817,7 +866,8 @@ def CIR_UnionType : CIR_Type<"Union", "union", [ OptionalParameter<"mlir::StringAttr">:$name, "bool":$incomplete, "bool":$packed, - OptionalParameter<"mlir::Type">:$padding + OptionalParameter<"mlir::Type">:$padding, + OptionalArrayRefParameter<"cir::RecordMemberKind">:$member_kinds ); // StorageClass is defined in C++ for mutability. @@ -833,27 +883,31 @@ def CIR_UnionType : CIR_Type<"Union", "union", [ "llvm::ArrayRef<mlir::Type>":$members, "mlir::StringAttr":$name, "bool":$packed, - CArg<"mlir::Type", "{}">:$padding + CArg<"mlir::Type", "{}">:$padding, + CArg<"llvm::ArrayRef<cir::RecordMemberKind>", "{}">:$member_kinds ), [{ return $_get($_ctxt, members, name, /*incomplete=*/false, packed, - padding); + padding, cir::normalizeRecordMemberKinds(member_kinds)); }]>, // Create an identified and incomplete union type. TypeBuilder<(ins "mlir::StringAttr":$name), [{ return $_get($_ctxt, /*members=*/llvm::ArrayRef<mlir::Type>{}, name, /*incomplete=*/true, /*packed=*/false, - /*padding=*/mlir::Type{}); + /*padding=*/mlir::Type{}, + /*member_kinds=*/llvm::ArrayRef<cir::RecordMemberKind>{}); }]>, // Create an anonymous union type (always complete). TypeBuilder<(ins "llvm::ArrayRef<mlir::Type>":$members, "bool":$packed, - CArg<"mlir::Type", "{}">:$padding + CArg<"mlir::Type", "{}">:$padding, + CArg<"llvm::ArrayRef<cir::RecordMemberKind>", "{}">:$member_kinds ), [{ return $_get($_ctxt, members, mlir::StringAttr{}, /*incomplete=*/false, - packed, padding); + packed, padding, + cir::normalizeRecordMemberKinds(member_kinds)); }]> ]; @@ -886,12 +940,15 @@ def CIR_UnionType : CIR_Type<"Union", "union", [ llvm::ArrayRef<mlir::Type> members); void complete(llvm::ArrayRef<mlir::Type> members, bool packed, - mlir::Type padding = {}); + mlir::Type padding = {}, + llvm::ArrayRef<cir::RecordMemberKind> memberKinds = {}); uint64_t getElementOffset(const mlir::DataLayout &, unsigned) const { return 0; } + /// Marks are provenance rather than layout, so two unions that differ only + /// in how their members were produced are layout-identical. bool isLayoutIdentical(const UnionType &other); // Checks the name of this record to check if it is a 'after' (or during) diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h b/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h index e94e1d81ff4c6..123fa059cfeac 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h +++ b/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h @@ -33,12 +33,14 @@ struct StructTypeStorage : public mlir::TypeStorage { bool incomplete; bool packed; bool padded; + llvm::ArrayRef<RecordMemberKind> member_kinds; bool is_class; KeyTy(llvm::ArrayRef<mlir::Type> members, mlir::StringAttr name, - bool incomplete, bool packed, bool padded, bool is_class) + bool incomplete, bool packed, bool padded, + llvm::ArrayRef<RecordMemberKind> member_kinds, bool is_class) : members(members), name(name), incomplete(incomplete), packed(packed), - padded(padded), is_class(is_class) {} + padded(padded), member_kinds(member_kinds), is_class(is_class) {} }; llvm::ArrayRef<mlir::Type> members; @@ -46,56 +48,73 @@ struct StructTypeStorage : public mlir::TypeStorage { bool incomplete; bool packed; bool padded; + llvm::ArrayRef<RecordMemberKind> member_kinds; bool is_class; StructTypeStorage(llvm::ArrayRef<mlir::Type> members, mlir::StringAttr name, - bool incomplete, bool packed, bool padded, bool is_class) + bool incomplete, bool packed, bool padded, + llvm::ArrayRef<RecordMemberKind> member_kinds, + bool is_class) : members(members), name(name), incomplete(incomplete), packed(packed), - padded(padded), is_class(is_class) { + padded(padded), member_kinds(member_kinds), is_class(is_class) { assert((name || !incomplete) && "Incomplete records must have a name"); + assert((member_kinds.empty() || member_kinds.size() == members.size()) && + "member kind list must cover every member"); } KeyTy getAsKey() const { - return KeyTy(members, name, incomplete, packed, padded, is_class); + return KeyTy(members, name, incomplete, packed, padded, member_kinds, + is_class); } bool operator==(const KeyTy &key) const { if (name) return (name == key.name) && (is_class == key.is_class); - return std::tie(members, name, incomplete, packed, padded, is_class) == - std::tie(key.members, key.name, key.incomplete, key.packed, - key.padded, key.is_class); + return std::tie(members, name, incomplete, packed, padded, member_kinds, + is_class) == std::tie(key.members, key.name, key.incomplete, + key.packed, key.padded, + key.member_kinds, key.is_class); } static llvm::hash_code hashKey(const KeyTy &key) { if (key.name) return llvm::hash_combine(key.name, key.is_class); return llvm::hash_combine(key.members, key.incomplete, key.packed, - key.padded, key.is_class); + key.padded, key.member_kinds, key.is_class); } static StructTypeStorage *construct(mlir::TypeStorageAllocator &allocator, const KeyTy &key) { - return new (allocator.allocate<StructTypeStorage>()) - StructTypeStorage(allocator.copyInto(key.members), key.name, - key.incomplete, key.packed, key.padded, key.is_class); + return new (allocator.allocate<StructTypeStorage>()) StructTypeStorage( + allocator.copyInto(key.members), key.name, key.incomplete, key.packed, + key.padded, allocator.copyInto(key.member_kinds), key.is_class); } /// Mutates the members and attributes of an identified struct/class. llvm::LogicalResult mutate(mlir::TypeStorageAllocator &allocator, llvm::ArrayRef<mlir::Type> members, bool packed, - bool padded) { + bool padded, + llvm::ArrayRef<RecordMemberKind> memberKinds) { if (!name) return llvm::failure(); + // A second completion must agree with the first in every parameter, + // including the marks: otherwise it silently keeps the marks it was given + // the first time. if (!incomplete) - return mlir::success((this->members == members) && - (this->packed == packed) && - (this->padded == padded)); + return mlir::success( + (this->members == members) && (this->packed == packed) && + (this->padded == padded) && (this->member_kinds == memberKinds)); + + // mutate is the one entrance verify() never sees, so check the length here + // rather than leave it to an assert. + if (!memberKinds.empty() && memberKinds.size() != members.size()) + return llvm::failure(); this->members = allocator.copyInto(members); this->packed = packed; this->padded = padded; + this->member_kinds = allocator.copyInto(memberKinds); incomplete = false; return llvm::success(); } @@ -113,11 +132,13 @@ struct UnionTypeStorage : public mlir::TypeStorage { bool incomplete; bool packed; mlir::Type padding; + llvm::ArrayRef<RecordMemberKind> member_kinds; KeyTy(llvm::ArrayRef<mlir::Type> members, mlir::StringAttr name, - bool incomplete, bool packed, mlir::Type padding) + bool incomplete, bool packed, mlir::Type padding, + llvm::ArrayRef<RecordMemberKind> member_kinds) : members(members), name(name), incomplete(incomplete), packed(packed), - padding(padding) {} + padding(padding), member_kinds(member_kinds) {} }; llvm::ArrayRef<mlir::Type> members; @@ -125,55 +146,69 @@ struct UnionTypeStorage : public mlir::TypeStorage { bool incomplete; bool packed; mlir::Type padding; + llvm::ArrayRef<RecordMemberKind> member_kinds; UnionTypeStorage(llvm::ArrayRef<mlir::Type> members, mlir::StringAttr name, - bool incomplete, bool packed, mlir::Type padding) + bool incomplete, bool packed, mlir::Type padding, + llvm::ArrayRef<RecordMemberKind> member_kinds) : members(members), name(name), incomplete(incomplete), packed(packed), - padding(padding) { + padding(padding), member_kinds(member_kinds) { assert((name || !incomplete) && "Incomplete records must have a name"); + assert((member_kinds.empty() || member_kinds.size() == members.size()) && + "member kind list must cover every member"); } KeyTy getAsKey() const { - return KeyTy(members, name, incomplete, packed, padding); + return KeyTy(members, name, incomplete, packed, padding, member_kinds); } bool operator==(const KeyTy &key) const { if (name) return name == key.name; - return std::tie(members, name, incomplete, packed, padding) == + return std::tie(members, name, incomplete, packed, padding, member_kinds) == std::tie(key.members, key.name, key.incomplete, key.packed, - key.padding); + key.padding, key.member_kinds); } static llvm::hash_code hashKey(const KeyTy &key) { if (key.name) return llvm::hash_combine(key.name); return llvm::hash_combine(key.members, key.incomplete, key.packed, - key.padding); + key.padding, key.member_kinds); } static UnionTypeStorage *construct(mlir::TypeStorageAllocator &allocator, const KeyTy &key) { - return new (allocator.allocate<UnionTypeStorage>()) - UnionTypeStorage(allocator.copyInto(key.members), key.name, - key.incomplete, key.packed, key.padding); + return new (allocator.allocate<UnionTypeStorage>()) UnionTypeStorage( + allocator.copyInto(key.members), key.name, key.incomplete, key.packed, + key.padding, allocator.copyInto(key.member_kinds)); } /// Mutates the members and attributes of an identified union. llvm::LogicalResult mutate(mlir::TypeStorageAllocator &allocator, llvm::ArrayRef<mlir::Type> members, bool packed, - mlir::Type padding) { + mlir::Type padding, + llvm::ArrayRef<RecordMemberKind> memberKinds) { if (!name) return llvm::failure(); + // A second completion must agree with the first in every parameter, + // including the marks: otherwise it silently keeps the marks it was given + // the first time. if (!incomplete) - return mlir::success((this->members == members) && - (this->packed == packed) && - (this->padding == padding)); + return mlir::success( + (this->members == members) && (this->packed == packed) && + (this->padding == padding) && (this->member_kinds == memberKinds)); + + // mutate is the one entrance verify() never sees, so check the length here + // rather than leave it to an assert. + if (!memberKinds.empty() && memberKinds.size() != members.size()) + return llvm::failure(); this->members = allocator.copyInto(members); this->packed = packed; this->padding = padding; + this->member_kinds = allocator.copyInto(memberKinds); incomplete = false; return llvm::success(); } diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp index af1bbdcd64fea..55fbba11ddb50 100644 --- a/cla... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/215174 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
