https://github.com/erichkeane created https://github.com/llvm/llvm-project/pull/210709
Perennial and a few other examples I came up with showed some situations where we were bad at layout in CIR for unions. Sometimes we'd omit the required fields, or miscalculate the storage type, or just not constant init it correctly. This is mostly because we copy/pasted our implementation from classic-codegen, but have different invariants. This patch consistently adds ALL fields to the cir::UnionType, so we always represent with all, which is an assumption we make elsewhere. The lowerUnion function is significantly rewritten as the previous version had a lot of interdepencies between loop runs. This patch separates it out in a way that has less cyclic complexity which will hopefully be more readable with minor/little compile-time impact. This maintains the non-virtual-base functionality as well, though it does it by collecting the fields like we do for the normal union case, then just erases them. This greatly simplifies the calculation of storageType, so it seems worth it. Additionally, it fixes the constant layout of a null union, which got the field count wrong in any non-trivial cases. >From 37b06ca3ca3456c7418d5d1a3fabd389c9bae66b Mon Sep 17 00:00:00 2001 From: erichkeane <[email protected]> Date: Fri, 17 Jul 2026 10:59:48 -0700 Subject: [PATCH] [CIR] Refactor lowerUnion so unions get laid out consistently Perennial and a few other examples I came up with showed some situations where we were bad at layout in CIR for unions. Sometimes we'd omit the required fields, or miscalculate the storage type, or just not constant init it correctly. This is mostly because we copy/pasted our implementation from classic-codegen, but have different invariants. This patch consistently adds ALL fields to the cir::UnionType, so we always represent with all, which is an assumption we make elsewhere. The lowerUnion function is significantly rewritten as the previous version had a lot of interdepencies between loop runs. This patch separates it out in a way that has less cyclic complexity which will hopefully be more readable with minor/little compile-time impact. This maintains the non-virtual-base functionality as well, though it does it by collecting the fields like we do for the normal union case, then just erases them. This greatly simplifies the calculation of storageType, so it seems worth it. Additionally, it fixes the constant layout of a null union, which got the field count wrong in any non-trivial cases. --- .../include/clang/CIR/Dialect/IR/CIRTypes.td | 2 + clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp | 2 +- .../CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp | 82 ++++++------- clang/lib/CIR/Dialect/IR/CIRTypes.cpp | 6 +- .../CIR/CodeGen/unions-with-zero-init.cpp | 116 ++++++++++++++++++ 5 files changed, 159 insertions(+), 49 deletions(-) create mode 100644 clang/test/CIR/CodeGen/unions-with-zero-init.cpp diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td index 9ed151392dc30..09c26af212f38 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td +++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td @@ -880,6 +880,8 @@ def CIR_UnionType : CIR_Type<"Union", "union", [ /// the storage type when lowering to LLVM IR). Returns a null type for /// empty unions. mlir::Type getUnionStorageType(const mlir::DataLayout &dataLayout) const; + static mlir::Type getUnionStorageType(const mlir::DataLayout &dataLayout, + llvm::ArrayRef<mlir::Type> members); void complete(llvm::ArrayRef<mlir::Type> members, bool packed, mlir::Type padding = {}); diff --git a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp index 4e16351c9a951..e8c14f2f5d40c 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp @@ -1044,7 +1044,7 @@ static mlir::TypedAttr emitNullConstant(CIRGenModule &cgm, const RecordDecl *rd, : layout.getBaseSubobjectCIRType()); auto recordTy = mlir::cast<cir::RecordType>(ty); - unsigned numElements = recordTy.getNumElements(); + unsigned numElements = rd->isUnion() ? 1 : recordTy.getNumElements(); SmallVector<mlir::Attribute> elements(numElements); auto *cxxrd = dyn_cast<CXXRecordDecl>(rd); diff --git a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp index 3b248ed9a0125..12a6ce8628d43 100644 --- a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp @@ -835,11 +835,10 @@ void CIRRecordLowering::lowerUnion(bool nonVirtualBaseType) { // an enclosing [[no_unique_address]] union field must use this smaller type. CharUnits layoutSize = nonVirtualBaseType ? astRecordLayout.getDataSize() : astRecordLayout.getSize(); - mlir::Type storageType = nullptr; - bool seenNamedMember = false; + // Accumulate bitfields and fields, and figure out what our difference between + // storage type and padding is. - // Iterate through the fields setting bitFieldInfo and the Fields array. Also - // locate the "most appropriate" storage type. + // First, accumulate all the types. for (const FieldDecl *field : recordDecl->fields()) { mlir::Type fieldType; if (field->isBitField()) { @@ -851,59 +850,50 @@ void CIRRecordLowering::lowerUnion(bool nonVirtualBaseType) { fieldType = getStorageType(field); } - // This maps a field to its index. For unions, the index is always 0. fieldIdxMap[field->getCanonicalDecl()] = 0; + fieldTypes.push_back(fieldType); + } - // Compute zero-initializable status. - // This union might not be zero initialized: it may contain a pointer to - // data member which might have some exotic initialization sequence. - // If this is the case, then we ought not to try and come up with a "better" - // type, it might not be very easy to come up with a Constant which - // correctly initializes it. - if (!seenNamedMember) { - seenNamedMember = field->getIdentifier(); - if (!seenNamedMember) - if (const RecordDecl *fieldRD = field->getType()->getAsRecordDecl()) - seenNamedMember = fieldRD->findFirstNamedDataMember(); - if (seenNamedMember && !isZeroInitializable(field)) { - zeroInitializable = zeroInitializableAsBase = false; - storageType = fieldType; - } - } + // Compute zero-initializable status. + // This union might not be zero initialized: it may contain a pointer to + // data member which might have some exotic initialization sequence. + // Unlike classic codegen, we don't really 'give up' on adding all the fields, + // though this is a decision/implementation we might want to revisit. We just + // fall back on the typical storage type calculation rather than this bizarre + // "choose the first thing", as that likely won't be compatible with later + // decisions. + for (const FieldDecl *field : recordDecl->fields()) { - // Because our union isn't zero initializable, we won't be getting a better - // storage type. - if (!zeroInitializable) - continue; + auto hasNamedMember = [](const FieldDecl *curField) -> bool { + const auto *rd = curField->getType()->getAsRecordDecl(); + return rd && rd->findFirstNamedDataMember(); + }; - // Conditionally update our storage type if we've got a new "better" one. - if (!storageType || getAlignment(fieldType) > getAlignment(storageType) || - (getAlignment(fieldType) == getAlignment(storageType) && - getSize(fieldType) > getSize(storageType))) - storageType = fieldType; - - // NOTE(cir): Track all union member's types, not just the largest one. It - // allows for proper type-checking and retain more info for analisys. - // - // The base-subobject type instead uses a single (possibly clipped) storage - // type, mirroring classic CodeGen, so that it exposes the union's reusable - // tail padding. - if (!nonVirtualBaseType) - fieldTypes.push_back(fieldType); + if ((field->getIdentifier() || hasNamedMember(field)) && + !isZeroInitializable(field)) { + zeroInitializable = zeroInitializableAsBase = false; + } } - if (!storageType) { + // If we have no candidates for storage, we are JUST padding. + if (fieldTypes.empty()) { appendPaddingBytes(layoutSize); return; } + mlir::Type storageType = + cir::UnionType::getUnionStorageType(dataLayout.layout, fieldTypes); + + // If our storage size was bigger than our required size (can happen in the + // case of packed bitfields on Itanium) then just use an I8 array. if (layoutSize < getSize(storageType)) storageType = getByteArrayType(layoutSize); + // The base-subobject record is built as a struct from fieldTypes, so add + // the storage type and any trailing padding as ordinary fields rather than + // routing padding through the union's single tail-padding slot. if (nonVirtualBaseType) { - // The base-subobject record is built as a struct from fieldTypes, so add - // the storage type and any trailing padding as ordinary fields rather than - // routing padding through the union's single tail-padding slot. + fieldTypes.clear(); fieldTypes.push_back(storageType); CharUnits padding = layoutSize - getSize(storageType); if (!padding.isZero()) { @@ -911,12 +901,10 @@ void CIRRecordLowering::lowerUnion(bool nonVirtualBaseType) { padded = true; } } else { + // Else we just add padding normally. appendPaddingBytes(layoutSize - getSize(storageType)); } - - // Set packed if we need it. - if (!layoutSize.isMultipleOf(getAlignment(storageType))) - packed = true; + packed = !layoutSize.isMultipleOf(getAlignment(storageType)); } bool CIRRecordLowering::hasOwnStorage(const CXXRecordDecl *decl, diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp index afd3364af4ec2..302790edc57cd 100644 --- a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp @@ -465,7 +465,11 @@ void UnionType::complete(ArrayRef<Type> members, bool packed, mlir::Type UnionType::getUnionStorageType(const mlir::DataLayout &dataLayout) const { - llvm::ArrayRef<mlir::Type> members = getMembers(); + return getUnionStorageType(dataLayout, getMembers()); +} + +mlir::Type UnionType::getUnionStorageType(const mlir::DataLayout &dataLayout, + llvm::ArrayRef<mlir::Type> members) { if (members.empty()) return {}; return *std::max_element( diff --git a/clang/test/CIR/CodeGen/unions-with-zero-init.cpp b/clang/test/CIR/CodeGen/unions-with-zero-init.cpp new file mode 100644 index 0000000000000..b3887635cacac --- /dev/null +++ b/clang/test/CIR/CodeGen/unions-with-zero-init.cpp @@ -0,0 +1,116 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -fclangir -Wno-unused-value -emit-cir -mmlir -mlir-print-ir-before=cir-cxxabi-lowering %s -o %t.cir 2> %t-before.cir +// RUN: FileCheck --check-prefix=CIR,CIR-BEFORE --input-file=%t-before.cir %s +// RUN: FileCheck --check-prefix=CIR,CIR-AFTER --input-file=%t.cir %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -fclangir -Wno-unused-value -emit-llvm %s -o %t-cir.ll +// RUN: FileCheck --check-prefix=LLVM,LLVMCIR --input-file=%t-cir.ll %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -Wno-unused-value -emit-llvm %s -o %t.ll +// RUN: FileCheck --check-prefix=LLVM,OGCG --input-file=%t.ll %s + +// 'S' doesn't end up in the 'after' IR because it loses its uses. +// CIR-BEFORE-DAG: !rec_S = !cir.struct<"S" {!s32i}> + +// CIR-BEFORE-DAG: !rec_inner_aggregate = !cir.union<"inner_aggregate" {!cir.data_member<!s32i in !rec_S>, !s32i}> +// CIR-AFTER-DAG: !rec_inner_aggregate = !cir.union<"inner_aggregate" {!s64i, !s32i}> +// LLVM-DAG: %union.inner_aggregate = type { i64 } + +// CIR-BEFORE-DAG: !rec_inner_aggregate2 = !cir.union<"inner_aggregate2" {!s32i, !cir.data_member<!s32i in !rec_S>}> +// CIR-AFTER-DAG: !rec_inner_aggregate2 = !cir.union<"inner_aggregate2" {!s32i, !s64i}> +// In LLVM, inner_aggregate2 was lowered to a literal, so the type went away. + +// CIR-BEFORE-DAG: !rec_outer_aggregate = !cir.union<"outer_aggregate" {!cir.data_member<!s32i in !rec_S>, !s32i}> +// CIR-AFTER-DAG: !rec_outer_aggregate = !cir.union<"outer_aggregate" {!s64i, !s32i}> +// LLVM-DAG: %union.outer_aggregate = type { i64 } + +// CIR-BEFORE-DAG: !rec_outer_aggregate2 = !cir.union<"outer_aggregate2" {!s32i, !cir.data_member<!s32i in !rec_S>}> +// CIR-AFTER-DAG: !rec_outer_aggregate2 = !cir.union<"outer_aggregate2" {!s32i, !s64i}> +// LLVM-DAG: %union.inner_aggregate2 = type { i64 } + +// CIR-BEFORE-DAG: !rec_outer_aggregate3 = !cir.union<"outer_aggregate3" {!cir.data_member<!s32i in !rec_S>, !s32i}> +// CIR-AFTER-DAG: !rec_outer_aggregate3 = !cir.union<"outer_aggregate3" {!s64i, !s32i}> +// LLVM-DAG: %union.outer_aggregate3 = type { i64 } + +// This gets promoted to a constant, so it is up here. +// CIR-AFTER-DAG: cir.global "private" constant cir_private @__const._Z1fv.inner_a2 = #cir.const_record<{#cir.int<12> : !s32i}> : !rec_inner_aggregate2 +// LLVM-DAG: @__const._Z1fv.inner_a2 = private {{.*}}constant { i32, [4 x i8] } { i32 12, [4 x i8] undef } + +struct S { int x; }; +int S::* p = nullptr; +// CIR-BEFORE-LABEL: cir.global external @p = #cir.data_member<null> : !cir.data_member<!s32i in !rec_S> +// CIR-AFTER-LABEL: cir.global external @p = #cir.int<-1> : !s64i +// LLVM-DAG: @p = global i64 -1, align 8 + +// LLVMCIR gets this different because by the time we see how to do a 'zero' +// field, we've already lost the member-pointer type, because LowerToLLVM is +// doing the 'zeroing'. We could be more clever here, but this is only in cases +// where it gets initialized anyway. +// LLVMCIR-DAG: @outer_a1 = global %union.outer_aggregate zeroinitializer +// OGCG-DAG: @outer_a1 = global %union.outer_aggregate { i64 -1 } + +// LLVM-DAG: @outer_a2 = global { i32, [4 x i8] } { i32 32, [4 x i8] undef } +// LLVM-DAG: @outer_a3 = global %union.outer_aggregate3 { i64 -1 } + +union outer_aggregate{int S::*m; int i; } outer_a1 = { p }; +// CIR-BEFORE-LABEL: cir.global external @outer_a1 = ctor : !rec_outer_aggregate { +// CIR-BEFORE: %[[GET_GLOB:.*]] = cir.get_global @outer_a1 : !cir.ptr<!rec_outer_aggregate> +// CIR-BEFORE: %[[GET_MEM:.*]] = cir.get_member %[[GET_GLOB]][0] {name = "m"} : !cir.ptr<!rec_outer_aggregate> -> !cir.ptr<!cir.data_member<!s32i in !rec_S>> +// CIR-BEFORE: %[[GET_P:.*]] = cir.get_global @p : !cir.ptr<!cir.data_member<!s32i in !rec_S>> +// CIR-BEFORE: %[[LOAD_P:.*]] = cir.load align(8) %[[GET_P]] : !cir.ptr<!cir.data_member<!s32i in !rec_S>>, !cir.data_member<!s32i in !rec_S> +// CIR-BEFORE: cir.store {{.*}}%[[LOAD_P]], %[[GET_MEM]] : !cir.data_member<!s32i in !rec_S>, !cir.ptr<!cir.data_member<!s32i in !rec_S>> +// CIR-BEFORE: } + +// CIR-AFTER-LABEL: cir.global external @outer_a1 = #cir.zero : !rec_outer_aggregate +// CIR-AFTER-LABEL: cir.func internal private @__cxx_global_var_init() { +// CIR-AFTER: %[[GET_GLOB:.*]] = cir.get_global @outer_a1 : !cir.ptr<!rec_outer_aggregate> +// CIR-AFTER: %[[GET_MEM:.*]] = cir.get_member %[[GET_GLOB]][0] {name = "m"} : !cir.ptr<!rec_outer_aggregate> -> !cir.ptr<!s64i> +// CIR-AFTER: %[[GET_P:.*]] = cir.get_global @p : !cir.ptr<!s64i> +// CIR-AFTER: %[[LOAD_P:.*]] = cir.load {{.*}}%[[GET_P]] : !cir.ptr<!s64i>, !s64i +// CIR-AFTER: cir.store {{.*}}%[[LOAD_P]], %[[GET_MEM]] : !s64i, !cir.ptr<!s64i> +// CIR-AFTER: cir.return +// CIR-AFTER: } + +// LLVM: define internal void @__cxx_global_var_init() +// LLVM: %[[LOAD_P:.*]] = load i64, ptr @p +// LLVM: store i64 %[[LOAD_P]], ptr @outer_a1 +// LLVM: ret void +// LLVM: } + +union outer_aggregate2{int i; int S::*m; } outer_a2 = { 32 }; +// CIR-LABEL: cir.global external @outer_a2 = #cir.const_record<{#cir.int<32> : !s32i}> : !rec_outer_aggregate2 +// LLVM version is above, because LLVM-IR orders vars before functions. + +union outer_aggregate3{int S::*m; int i; } outer_a3; +// CIR-BEFORE-LABEL: cir.global external @outer_a3 = #cir.const_record<{#cir.data_member<null> : !cir.data_member<!s32i in !rec_S>}> : !rec_outer_aggregate3 +// CIR-AFTER-LABEL: cir.global external @outer_a3 = #cir.const_record<{#cir.int<-1> : !s64i}> : !rec_outer_aggregate3 +// LLVM version is above, because LLVM-IR orders vars before functions. + +void f() { + union inner_aggregate{int S::*m; int i; } inner_a1 = { p }; + union inner_aggregate2{int i; int S::*m; } inner_a2 = { 12 }; +} +// CIR-LABEL: cir.func {{.*}}@_Z1fv() +// CIR: %[[A1_ALLOCA:.*]] = cir.alloca "inner_a1" {{.*}}init : !cir.ptr<!rec_inner_aggregate> +// CIR: %[[A2_ALLOCA:.*]] = cir.alloca "inner_a2" {{.*}}init : !cir.ptr<!rec_inner_aggregate2> +// CIR-BEFORE: %[[GET_A1_M:.*]] = cir.get_member %[[A1_ALLOCA]][0] {name = "m"} : !cir.ptr<!rec_inner_aggregate> -> !cir.ptr<!cir.data_member<!s32i in !rec_S>> +// CIR-AFTER: %[[GET_A1_M:.*]] = cir.get_member %[[A1_ALLOCA]][0] {name = "m"} : !cir.ptr<!rec_inner_aggregate> -> !cir.ptr<!s64i> +// CIR-BEFORE: %[[GET_P:.*]] = cir.get_global @p : !cir.ptr<!cir.data_member<!s32i in !rec_S>> +// CIR-AFTER: %[[GET_P:.*]] = cir.get_global @p : !cir.ptr<!s64i> +// CIR-BEFORE: %[[LOAD_P:.*]] = cir.load {{.*}}%[[GET_P]] : !cir.ptr<!cir.data_member<!s32i in !rec_S>>, !cir.data_member<!s32i in !rec_S> +// CIR-AFTER: %[[LOAD_P:.*]] = cir.load {{.*}}%[[GET_P]] : !cir.ptr<!s64i>, !s64i +// CIR-BEFORE: cir.store {{.*}}%[[LOAD_P]], %[[GET_A1_M]] : !cir.data_member<!s32i in !rec_S>, !cir.ptr<!cir.data_member<!s32i in !rec_S>> +// CIR-AFTER: cir.store {{.*}}%[[LOAD_P]], %[[GET_A1_M]] : !s64i, !cir.ptr<!s64i> +// A2 is converted to a global constant, so there is slightly different behaviors here. +// CIR-BEFORE: %[[TWELVE:.*]] = cir.const #cir.const_record<{#cir.int<12> : !s32i}> : !rec_inner_aggregate2 +// CIR-BEFORE: cir.store {{.*}}%[[TWELVE]], %[[A2_ALLOCA]] : !rec_inner_aggregate2, !cir.ptr<!rec_inner_aggregate2> +// CIR-AFTER: %[[GET_A2_GLOB:.*]] = cir.get_global @__const._Z1fv.inner_a2 : !cir.ptr<!rec_inner_aggregate2> +// CIR-AFTER: cir.copy %[[GET_A2_GLOB]] to %[[A2_ALLOCA]] : !cir.ptr<!rec_inner_aggregate2> +// CIR: cir.return +// CIR: } + +// LLVM-LABEL: define dso_local void @_Z1fv() +// LLVM: %[[A1_ALLOCA:.*]] = alloca %union.inner_aggregate +// LLVM: %[[A2_ALLOCA:.*]] = alloca %union.inner_aggregate2 +// LLVM: %[[LOAD_P:.*]] = load i64, ptr @p +// LLVM: store i64 %[[LOAD_P]], ptr %[[A1_ALLOCA]] +// LLVM: call void @llvm.memcpy.p0.p0.i64(ptr {{.*}}%[[A2_ALLOCA]], ptr {{.*}}@__const._Z1fv.inner_a2, i64 8, i1 false) +// LLVM: ret void +// LLVM: } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
