llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Adam Smith (adams381) <details> <summary>Changes</summary> Passing a union to a function does not work. The x86_64 bridge rejects every one, so the pass fails on any signature naming a union. Additionally, indirect arguments have their byval and sret alignment wrong, because `mapCIRType` asks DataLayout for it, and DataLayout only sees a record's members, never `__attribute__((aligned(N)))`. Unions now go through the ABI library's union type, which puts every member at offset zero and sizes each eightbyte from the union rather than from a single member. The alignment comes from the record-layout metadata the AST already fills in, which fixes over-aligned structs too, since they share that lookup. Assisted-by: Cursor / claude-opus-5 --- Patch is 39.85 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/214129.diff 7 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIRDialect.h (+5) - (modified) clang/lib/CIR/Dialect/IR/CIRAttrs.cpp (+12-4) - (modified) clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp (+79-44) - (modified) clang/test/CIR/CodeGen/call-conv-lowering-x86_64.c (+84) - (modified) clang/test/CIR/Transforms/abi-lowering/x86_64-aggregate-nyi.cir (+77-5) - (added) clang/test/CIR/Transforms/abi-lowering/x86_64-union-coerce-shapes.cir (+80) - (added) clang/test/CIR/Transforms/abi-lowering/x86_64-union.cir (+280) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h index c6f6c80206bfe..8cd70bf47ce5a 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h +++ b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h @@ -84,6 +84,11 @@ class FenvOpTrait : public mlir::OpTrait::TraitBase<ConcreteType, FenvOpTrait> { /// Look up the RecordLayoutAttr for a named record in the module's /// cir.record_layouts dictionary. Asserts if the entry is missing. RecordLayoutAttr getRecordLayout(mlir::ModuleOp module, mlir::StringAttr name); + +/// Same lookup as getRecordLayout, but returns a null attribute instead of +/// asserting when the record has no layout entry. +RecordLayoutAttr tryGetRecordLayout(mlir::ModuleOp module, + mlir::StringAttr name); } // namespace cir // TableGen'erated files for MLIR dialects require that a macro be defined when diff --git a/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp b/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp index 264e836718c81..74a3deb5b2508 100644 --- a/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp @@ -920,12 +920,20 @@ LogicalResult DynamicCastInfoAttr::verify( // RecordLayout lookup //===----------------------------------------------------------------------===// -RecordLayoutAttr cir::getRecordLayout(mlir::ModuleOp module, - mlir::StringAttr name) { +RecordLayoutAttr cir::tryGetRecordLayout(mlir::ModuleOp module, + mlir::StringAttr name) { + if (!name) + return {}; auto dict = module->getAttrOfType<mlir::DictionaryAttr>( CIRDialect::getRecordLayoutsAttrName()); - assert(dict && "module missing cir.record_layouts attribute"); - auto attr = dict.getAs<RecordLayoutAttr>(name); + if (!dict) + return {}; + return dict.getAs<RecordLayoutAttr>(name); +} + +RecordLayoutAttr cir::getRecordLayout(mlir::ModuleOp module, + mlir::StringAttr name) { + RecordLayoutAttr attr = tryGetRecordLayout(module, name); assert(attr && "record layout entry missing for named record"); return attr; } diff --git a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp index 54c4487db6195..1baa145251bc9 100644 --- a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp +++ b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp @@ -68,10 +68,11 @@ namespace { // SysV x86_64 classifier, and converts the result back into the // dialect-agnostic mlir::abi::FunctionClassification that CIRABIRewriteContext // consumes. Integer (including `_BitInt` up to 128 bits) / pointer / bool / -// f32 / f64 scalars and struct / array aggregates are handled. Unions, -// `_Complex`, vectors, wider floats, and packed or padded records are reported -// NYI by classifyX86_64Function so an unsupported signature fails the pass -// instead of being misclassified. +// f32 / f64 scalars and struct / union / array aggregates are handled. +// `_Complex`, vectors, wider floats, packed or padded records, and a union no +// member of which spans its declared size are reported NYI by +// classifyX86_64Function so an unsupported signature fails the pass instead of +// being misclassified. //===----------------------------------------------------------------------===// /// Whether a struct's declared argument-passing kind (from the module's @@ -79,27 +80,32 @@ namespace { /// no layout entry (e.g. an anonymous struct) has no C++ non-trivial reason to /// be forced to memory, so it defaults to can-pass-in-registers. static bool recordCanPassInRegs(ModuleOp modOp, cir::RecordType recTy) { - mlir::StringAttr name = recTy.getName(); - if (!name) - return true; - auto dict = modOp->getAttrOfType<DictionaryAttr>( - cir::CIRDialect::getRecordLayoutsAttrName()); - if (!dict) - return true; - auto layout = dict.getAs<cir::RecordLayoutAttr>(name); + auto layout = cir::tryGetRecordLayout(modOp, recTy.getName()); if (!layout) return true; return layout.getArgPassingKind() == cir::ArgPassingKind::CanPassInRegs; } +/// A record's declared alignment, which the ABI uses for the byval and sret +/// alignment of an indirect argument. DataLayout derives alignment from the +/// members, so it cannot see `__attribute__((aligned(N)))`. The declared value +/// comes from the module's record-layout metadata instead. CIRGen emits an +/// entry for every record it names, so the computed fallback only serves +/// hand-written CIR. +static llvm::Align recordDeclaredAlign(ModuleOp modOp, cir::RecordType recTy, + const DataLayout &dl) { + auto layout = cir::tryGetRecordLayout(modOp, recTy.getName()); + if (!layout) + return llvm::Align(dl.getTypeABIAlignment(recTy)); + return llvm::Align(layout.getRecordAlign()); +} + /// The CIR types the x86_64 bridge handles. Scalars: an integer up to 128 /// bits (including `_BitInt` and `__int128`), pointer, bool, void, f32, or f64. -/// Aggregates: a complete struct whose fields are all themselves supported, or -/// an array of a supported element type. A `_BitInt` wider than 128 bits, -/// unions, `_Complex`, vectors, wider floats, and packed or padded records are -/// not handled and are reported NYI at the reject() choke point in -/// classifyX86_64Function. -static bool isSupportedType(mlir::Type ty) { +/// Aggregates: a complete struct or union whose members are all themselves +/// supported, or an array of a supported element type. Everything else is +/// reported NYI at the reject() choke point in classifyX86_64Function. +static bool isSupportedType(mlir::Type ty, const DataLayout &dl) { // A pointer is only handled in the default address space (null) or an // already-lowered target address space. A LangAddressSpaceAttr must be // lowered before this pass, so reject it rather than silently dropping it. @@ -124,21 +130,40 @@ static bool isSupportedType(mlir::Type ty) { return intTy.getWidth() <= 64 || intTy.getWidth() == 128; } if (auto arrTy = dyn_cast<cir::ArrayType>(ty)) - return isSupportedType(arrTy.getElementType()); + return isSupportedType(arrTy.getElementType(), dl); if (auto recTy = dyn_cast<cir::RecordType>(ty)) { - // Unions and packed / padded records each need classification this bridge - // does not implement (a union widen fixup and pad-aware eightbyte - // classification), so reject them here and report NYI rather than - // misclassify. A zero-field record (a C empty struct) classifies as - // Ignore and is dropped from the lowered signature. CIRGen lays out an - // empty C++ class as a single padded byte, which the padded check rejects. - // A real one-byte struct such as `{char[1]}` has a field and is not - // padded, so it is classified normally. - if (recTy.isUnion() || !recTy.isComplete() || recTy.getPacked() || - recTy.getPadded()) + // An incomplete record has no layout to classify, and a packed one needs + // pad-aware eightbyte classification this bridge does not implement. + if (!recTy.isComplete() || recTy.getPacked()) return false; + if (recTy.isUnion()) { + // The classifier sizes a union's eightbytes from the union itself, which + // is only sound when some member spans that size. Short of that, the + // remaining bytes are either tail padding or the rest of a bitfield + // storage unit, and the CIR type cannot tell those apart even though + // classic CodeGen coerces them to i32 and i8 respectively. + llvm::ArrayRef<mlir::Type> members = recTy.getMembers(); + uint64_t recordBits = dl.getTypeSizeInBits(recTy).getFixedValue(); + if (members.empty()) { + // A member-less union is all padding, which classifies Ignore up to two + // eightbytes. Past that SysV says MEMORY regardless of content, and + // there is no member here to build the Indirect coercion from. + if (recordBits > 128) + return false; + } else { + auto spansRecord = [&](mlir::Type m) { + return dl.getTypeSizeInBits(m).getFixedValue() == recordBits; + }; + if (!llvm::any_of(members, spansRecord)) + return false; + } + } else if (recTy.getPadded()) { + // A struct's padding is a member the classifier would have to recognize + // as padding rather than data, which is not implemented. + return false; + } return llvm::all_of(recTy.getMembers(), - [](mlir::Type m) { return isSupportedType(m); }); + [&](mlir::Type m) { return isSupportedType(m, dl); }); } return false; } @@ -220,11 +245,28 @@ static const llvm::abi::Type *mapCIRType(mlir::Type type, dl.getTypeSizeInBits(type).getFixedValue()); }) .Case([&](cir::RecordType recTy) -> const llvm::abi::Type * { - // isSupportedType rejects unions, packed / padded, and empty-for-ABI - // records, so this handles a plain struct: map each field at its - // naturally-aligned offset. + llvm::abi::RecordFlags flags = llvm::abi::RecordFlags::None; + if (recordCanPassInRegs(modOp, recTy)) + flags = flags | llvm::abi::RecordFlags::CanPassInRegisters; + llvm::TypeSize sizeBits = llvm::TypeSize::getFixed( + dl.getTypeSizeInBits(type).getFixedValue()); + llvm::Align align = recordDeclaredAlign(modOp, recTy, dl); SmallVector<llvm::abi::FieldInfo> fields; fields.reserve(recTy.getMembers().size()); + + // The size passed here spans the tail padding, so an eightbyte covers + // the whole union rather than just the member the classifier reduces + // it to. + if (recTy.isUnion()) { + for (mlir::Type fieldTy : recTy.getMembers()) + fields.push_back(llvm::abi::FieldInfo( + mapCIRType(fieldTy, typeMapper, dl, modOp))); + return tb.getUnionType(fields, sizeBits, align, + llvm::abi::StructPacking::Default, flags); + } + + // isSupportedType rejects packed and padded structs, so every field + // here sits at its naturally-aligned offset. uint64_t offsetBits = 0; for (mlir::Type fieldTy : recTy.getMembers()) { const llvm::abi::Type *mappedField = @@ -234,16 +276,9 @@ static const llvm::abi::Type *mapCIRType(mlir::Type type, fields.push_back(llvm::abi::FieldInfo(mappedField, offsetBits)); offsetBits += dl.getTypeSizeInBits(fieldTy).getFixedValue(); } - llvm::abi::RecordFlags flags = llvm::abi::RecordFlags::None; - if (recordCanPassInRegs(modOp, recTy)) - flags = flags | llvm::abi::RecordFlags::CanPassInRegisters; - return tb.getRecordType(fields, - llvm::TypeSize::getFixed( - dl.getTypeSizeInBits(type).getFixedValue()), - llvm::Align(dl.getTypeABIAlignment(type)), - llvm::abi::StructPacking::Default, - /*BaseClasses=*/{}, /*VirtualBaseClasses=*/{}, - flags); + return tb.getRecordType( + fields, sizeBits, align, llvm::abi::StructPacking::Default, + /*BaseClasses=*/{}, /*VirtualBaseClasses=*/{}, flags); }) .Default([](mlir::Type) -> const llvm::abi::Type * { llvm_unreachable( @@ -388,7 +423,7 @@ static std::optional<FunctionClassification> classifyX86_64Signature( bool voidRet = isa<cir::VoidType>(retCIR); auto reject = [&](mlir::Type t) -> bool { - if (isSupportedType(t)) + if (isSupportedType(t, dl)) return false; emitError() << "x86_64 calling-convention lowering not yet implemented for type " diff --git a/clang/test/CIR/CodeGen/call-conv-lowering-x86_64.c b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64.c index 6931198a90c3d..8382b15bb5d9e 100644 --- a/clang/test/CIR/CodeGen/call-conv-lowering-x86_64.c +++ b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64.c @@ -11,6 +11,11 @@ typedef struct { long a, b, c, d; } Big; typedef struct { long a; double b; } IntSSE; typedef struct { double a; double b; } SSE2; typedef struct { } Empty; +typedef union { int i; float f; } UIntFloat; +typedef union { float f; float g; } UFloats; +typedef union { int i; char c[8]; } UNarrowStorage; +typedef union { char c[32]; } UBig; +typedef union { char c[32]; } __attribute__((aligned(32))) UBigOverAligned; // Narrow signed integer sign-extended in a register. signed char ext_schar(signed char c) { return c; } @@ -79,3 +84,82 @@ void take_big(Big b) { (void)b; } // CIR: cir.func {{.*}}@take_big(%arg0: !cir.ptr<!rec_Big> {{.*}}llvm.byval = !rec_Big{{.*}}) // LLVM-CIR: define dso_local void @take_big(ptr noalias noundef byval(%struct.Big) align 8 %{{.+}}) // LLVM-OGCG: define dso_local void @take_big(ptr noundef byval(%struct.Big) align 8 %{{.+}}) + +// Union members all start at offset zero, so a 4-byte union takes one INTEGER +// eightbyte and coerces to i32. +void take_union(UIntFloat u) { (void)u; } + +// CIR: cir.func {{.*}}@take_union(%arg0: !s32i{{.*}}) +// LLVM: define dso_local void @take_union(i32 %{{.+}}) + +// A union of floats classifies SSE, so it coerces to a float register. +void take_union_floats(UFloats u) { (void)u; } + +// CIR: cir.func {{.*}}@take_union_floats(%arg0: !cir.float{{.*}}) +// LLVM: define dso_local void @take_union_floats(float %{{.+}}) + +// The union's highest-aligned member is the 4-byte int, but its size comes +// from the 8-byte array, and the eightbyte is sized from the union. +void take_union_narrow_storage(UNarrowStorage u) { (void)u; } + +// CIR: cir.func {{.*}}@take_union_narrow_storage(%arg0: !u64i{{.*}}) +// LLVM: define dso_local void @take_union_narrow_storage(i64 %{{.+}}) + +// A coerced union return round-trips through the coercion type. +UIntFloat ret_union(int a) { UIntFloat u; u.i = a; return u; } + +// CIR: cir.func {{.*}}@ret_union(%arg0: !s32i {{.*}}) -> !s32i +// LLVM: define dso_local i32 @ret_union(i32 noundef %{{.+}}) + +// A union too large for registers is passed byval, with the same noalias +// divergence as a large struct. +void take_union_big(UBig u) { (void)u; } + +// CIR: cir.func {{.*}}@take_union_big(%arg0: !cir.ptr<!rec_UBig> {{.*}}llvm.byval = !rec_UBig{{.*}}) +// LLVM-CIR: define dso_local void @take_union_big(ptr noalias noundef byval(%union.UBig) align 8 %{{.+}}) +// LLVM-OGCG: define dso_local void @take_union_big(ptr noundef byval(%union.UBig) align 8 %{{.+}}) + +// The byval alignment follows the union's declared alignment, not the alignment +// its members imply, which is 1 here. +void take_union_big_over_aligned(UBigOverAligned u) { (void)u; } + +// CIR: cir.func {{.*}}@take_union_big_over_aligned(%arg0: !cir.ptr<!rec_UBigOverAligned> {{.*}}llvm.align = 32 : i64{{.*}}llvm.byval = !rec_UBigOverAligned{{.*}}) +// LLVM-CIR: define dso_local void @take_union_big_over_aligned(ptr noalias noundef byval(%union.UBigOverAligned) align 32 %{{.+}}) +// LLVM-OGCG: define dso_local void @take_union_big_over_aligned(ptr noundef byval(%union.UBigOverAligned) align 32 %{{.+}}) + +void call_union(UIntFloat u) { take_union(u); } + +// CIR: cir.func {{.*}}@call_union(%arg0: !s32i +// CIR: cir.call @take_union(%{{.+}}) : (!s32i) -> () +// LLVM: define dso_local void @call_union(i32 %{{.+}}) +// LLVM: call void @take_union(i32 %{{.+}}) + +void call_union_big_over_aligned(UBigOverAligned u) { + take_union_big_over_aligned(u); +} + +// CIR: cir.func {{.*}}@call_union_big_over_aligned(%arg0: !cir.ptr<!rec_UBigOverAligned> {{.*}}llvm.align = 32 : i64{{.*}}) +// CIR: cir.call @take_union_big_over_aligned(%{{.+}}) : (!cir.ptr<!rec_UBigOverAligned> {{.*}}llvm.align = 32 : i64{{.*}}) -> () +// LLVM-CIR: define dso_local void @call_union_big_over_aligned(ptr noalias noundef byval(%union.UBigOverAligned) align 32 %{{.+}}) +// LLVM-CIR: alloca %union.UBigOverAligned, i64 1, align 32 +// LLVM-CIR: call void @take_union_big_over_aligned(ptr noalias noundef byval(%union.UBigOverAligned) align 32 %{{.+}}) +// LLVM-OGCG: define dso_local void @call_union_big_over_aligned(ptr noundef byval(%union.UBigOverAligned) align 32 %{{.+}}) +// LLVM-OGCG: call void @take_union_big_over_aligned(ptr noundef byval(%union.UBigOverAligned) align 32 %{{.+}}) + +// The declared alignment reaches the sret slot of an indirect return too, not +// just a byval argument. +UBigOverAligned ret_union_big_over_aligned(void); +void call_ret_union_big_over_aligned(void) { (void)ret_union_big_over_aligned(); } + +// CIR: cir.func {{.*}}@ret_union_big_over_aligned(!cir.ptr<!rec_UBigOverAligned> {{.*}}llvm.align = 32 : i64{{.*}}llvm.sret = !rec_UBigOverAligned{{.*}}) +// LLVM: declare void @ret_union_big_over_aligned(ptr dead_on_unwind writable sret(%union.UBigOverAligned) align 32) + +// The same declared-alignment source feeds an over-aligned struct, since +// mapCIRType's alignment lookup is on the shared record path, not a +// union-specific one. +typedef struct { char c[32]; } __attribute__((aligned(32))) SOverAligned; +void take_struct_over_aligned(SOverAligned s) { (void)s; } + +// CIR: cir.func {{.*}}@take_struct_over_aligned(%arg0: !cir.ptr<!rec_SOverAligned> {{.*}}llvm.align = 32 : i64{{.*}}llvm.byval = !rec_SOverAligned{{.*}}) +// LLVM-CIR: define dso_local void @take_struct_over_aligned(ptr noalias noundef byval(%struct.SOverAligned) align 32 %{{.+}}) +// LLVM-OGCG: define dso_local void @take_struct_over_aligned(ptr noundef byval(%struct.SOverAligned) align 32 %{{.+}}) diff --git a/clang/test/CIR/Transforms/abi-lowering/x86_64-aggregate-nyi.cir b/clang/test/CIR/Transforms/abi-lowering/x86_64-aggregate-nyi.cir index 85f05fef9f96b..83d36ce22b970 100644 --- a/clang/test/CIR/Transforms/abi-lowering/x86_64-aggregate-nyi.cir +++ b/clang/test/CIR/Transforms/abi-lowering/x86_64-aggregate-nyi.cir @@ -1,10 +1,17 @@ // RUN: not cir-opt %s -cir-call-conv-lowering=target=x86_64 2>&1 | FileCheck %s !s8i = !cir.int<s, 8> +!s16i = !cir.int<s, 16> !s32i = !cir.int<s, 32> !u8i = !cir.int<u, 8> -!u32i = !cir.int<u, 32> -!rec_U = !cir.union<"U" {!s32i, !u32i}> +!rec_UPacked = !cir.union<"UPacked" packed {!s32i, !cir.array<!s8i x 5>}, padding = {!u8i}> +!rec_ULongDouble = !cir.union<"ULongDouble" {!cir.long_double<!cir.f80>, !s32i}> +!rec_UFloats = !cir.union<"UFloats" {!cir.array<!cir.float x 2>, !cir.array<!cir.float x 2>}> +!rec_UOverAligned = !cir.union<"UOverAligned" {!s32i}, padding = {!cir.array<!u8i x 12>}> +!rec_UShortStorage = !cir.union<"UShortStorage" {!s16i, !cir.array<!s8i x 3>}, padding = {!cir.array<!u8i x 2>}> +!rec_UByteBlobs = !cir.union<"UByteBlobs" {!u8i, !u8i}, padding = {!cir.array<!u8i x 3>}> +!rec_SWrapsOverAligned = !cir.struct<"SWrapsOverAligned" {!cir.double, !rec_UOverAligned}> +!rec_UEmptyLarge = !cir.union<"UEmptyLarge" {}, padding = {!cir.array<!u8i x 32>}> !rec_P = !cir.struct<"P" packed {!s8i, !s32i}> !rec_Ov = !cir.struct<"Ov" padded {!s32i, !cir.array<!u8i x 12>}> !rec_E = !cir.struct<"E" padded {!u8i}> @@ -19,12 +26,77 @@ module attributes { #dlti.dl_entry<f64, dense<64>: vector<2xi64>>> } { - // A union is rejected: its register coercion needs a widen fixup. - cir.func @take_union(%arg0: !rec_U) { + // A packed union is rejected for the same reason a packed struct is: its + // members no longer sit at their natural alignment. + cir.func @take_packed_union(%arg0: !rec_UPacked) { cir.return } - // CHECK: not yet implemented for type '!cir.union<"U" + // CHECK: not yet implemented for type '!cir.union<"UPacked" packed + + // A union member the bridge does not map keeps the whole union unsupported. + cir.func @take_union_long_double(%arg0: !rec_ULongDouble) { + cir.return + } + + // CHECK: not yet implemented for type '!cir.union<"ULongDouble" + + // A union whose highest-aligned member is an all-float array classifies to + // an SSE vector coerce this bridge does not represent, so it is reported NYI + // rather than passed unchanged. + cir.func @take_union_float_arrays(%arg0: !rec_UFloats) { + cir.return + } + + // CHECK: not yet implemented for the ABI coercion of type '!cir.union<"UFloats... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/214129 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
