https://github.com/adams381 created https://github.com/llvm/llvm-project/pull/218532
While implementing union support for ClangIR, I found that `reduceUnionForX8664` can choose a member holding no data as the type representing the union. It chooses by alignment and then by size, and an over-aligned empty class or an array of empty records wins either comparison, so the coercion comes out wider than Clang's. Clang does not need this skip because it compares lowered types, where an empty class is a byte array it can narrow the coercion through. A record reaching this library has no fields, so there is nothing to narrow through. This change skips members holding no data, using `bitsContainNoUserData` to identify them. Assisted-by: Cursor / claude-opus-5 >From d0eb80ac964f02a8698c9ccaec26010199deed41 Mon Sep 17 00:00:00 2001 From: Adam Smith <[email protected]> Date: Mon, 24 Aug 2026 14:34:23 -0700 Subject: [PATCH] [LLVMABI] Skip data-free members when reducing a union While implementing union support for ClangIR, I found that `reduceUnionForX8664` can choose a member holding no data as the type representing the union. It chooses by alignment and then by size, and an over-aligned empty class or an array of empty records wins either comparison, so the coercion comes out wider than Clang's. Clang does not need this skip because it compares lowered types, where an empty class is a byte array it can narrow the coercion through. A record reaching this library has no fields, so there is nothing to narrow through. This change skips members holding no data, using `bitsContainNoUserData` to identify them. Assisted-by: Cursor / claude-opus-5 --- .../X86/x86_64-union-empty-member-abi.cpp | 41 +++ llvm/lib/ABI/Targets/X86.cpp | 13 + llvm/unittests/ABI/CMakeLists.txt | 1 + llvm/unittests/ABI/X86TargetInfoTest.cpp | 235 ++++++++++++++++++ 4 files changed, 290 insertions(+) create mode 100644 clang/test/CodeGen/X86/x86_64-union-empty-member-abi.cpp create mode 100644 llvm/unittests/ABI/X86TargetInfoTest.cpp diff --git a/clang/test/CodeGen/X86/x86_64-union-empty-member-abi.cpp b/clang/test/CodeGen/X86/x86_64-union-empty-member-abi.cpp new file mode 100644 index 0000000000000..5742a44444e3e --- /dev/null +++ b/clang/test/CodeGen/X86/x86_64-union-empty-member-abi.cpp @@ -0,0 +1,41 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -emit-llvm %s -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -emit-llvm -fexperimental-abi-lowering %s -o - | FileCheck %s + +struct Empty {}; +struct alignas(16) EmptyAligned {}; + +extern "C" { + +// The empty member's alignment outranks the int's, so it wins the union's +// storage-type comparison and the coercion widens to i64 unless members that +// supply no bytes are skipped. +union OverAligned { EmptyAligned e; int i; }; +void take_over_aligned(union OverAligned u); +void call_over_aligned(union OverAligned u) { take_over_aligned(u); } +// CHECK-DAG: declare void @take_over_aligned(i32) + +// At equal alignment the comparison falls to size, which an array of empty +// records wins without supplying any bytes. +union ArrOfEmpty { Empty a[2]; char c; }; +void take_arr_of_empty(union ArrOfEmpty u); +void call_arr_of_empty(union ArrOfEmpty u) { take_arr_of_empty(u); } +// CHECK-DAG: declare void @take_arr_of_empty(i8) + +// The same array spanning a whole eightbyte. +union Arr8OfEmpty { Empty a[8]; char c; }; +void take_arr8_of_empty(union Arr8OfEmpty u); +void call_arr8_of_empty(union Arr8OfEmpty u) { take_arr8_of_empty(u); } +// CHECK-DAG: declare void @take_arr8_of_empty(i8) + +// Where the other member does fill the eightbyte there is nothing to narrow. +union EmptyAndBytes { Empty e; char c[8]; }; +void take_empty_and_bytes(union EmptyAndBytes u); +void call_empty_and_bytes(union EmptyAndBytes u) { take_empty_and_bytes(u); } +// CHECK-DAG: declare void @take_empty_and_bytes(i64) + +// Skipping the empty member leaves the remaining member's class intact. +union EmptyAndDouble { Empty e; double d; }; +void take_empty_and_double(union EmptyAndDouble u); +void call_empty_and_double(union EmptyAndDouble u) { take_empty_and_double(u); } +// CHECK-DAG: declare void @take_empty_and_double(double) +} diff --git a/llvm/lib/ABI/Targets/X86.cpp b/llvm/lib/ABI/Targets/X86.cpp index 43e30c5a43a18..63045ebe225a6 100644 --- a/llvm/lib/ABI/Targets/X86.cpp +++ b/llvm/lib/ABI/Targets/X86.cpp @@ -121,6 +121,9 @@ class X86_64TargetInfo : public TargetInfo { bool has64BitPointers() const { return Has64BitPointers; } }; +static bool bitsContainNoUserData(const Type *Ty, unsigned StartBit, + unsigned EndBit); + // Gets the "best" type to represent the union. static const Type *reduceUnionForX8664(const RecordType *UnionType, TypeBuilder &TB) { @@ -146,6 +149,16 @@ static const Type *reduceUnionForX8664(const RecordType *UnionType, break; } + // A member that holds no user data supplies no bytes for a coercion to + // read, so it must not become the storage type however wide or aligned it + // is declared. Clang compares lowered types instead, where an empty class + // is a byte array whose i8 leaf lets getIntegerTypeAtOffset narrow the + // coercion. A record mapped here holds no fields, so there is no such + // leaf and the eightbyte would be sized from the union. + if (bitsContainNoUserData(FieldType, 0, + FieldType->getSizeInBits().getFixedValue())) + continue; + if (!StorageType || FieldType->getAlignment() > StorageType->getAlignment() || (FieldType->getAlignment() == StorageType->getAlignment() && diff --git a/llvm/unittests/ABI/CMakeLists.txt b/llvm/unittests/ABI/CMakeLists.txt index fe0431524c7c2..a26da474bd83a 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 + X86TargetInfoTest.cpp ) diff --git a/llvm/unittests/ABI/X86TargetInfoTest.cpp b/llvm/unittests/ABI/X86TargetInfoTest.cpp new file mode 100644 index 0000000000000..64b918993d6d3 --- /dev/null +++ b/llvm/unittests/ABI/X86TargetInfoTest.cpp @@ -0,0 +1,235 @@ +//===- X86TargetInfoTest.cpp - x86 ABI 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/FunctionInfo.h" +#include "llvm/ABI/TargetInfo.h" +#include "llvm/ABI/Types.h" +#include "llvm/ADT/APFloat.h" +#include "llvm/IR/CallingConv.h" +#include "llvm/Support/Alignment.h" +#include "llvm/Support/Allocator.h" +#include "gtest/gtest.h" + +namespace { + +// RecordFlags' bitmask operators are declared in namespace llvm, so combining +// two of them needs that namespace visible. +using namespace llvm; + +using ABIType = llvm::abi::Type; +using llvm::abi::ABICompatInfo; +using llvm::abi::ArgInfo; +using llvm::abi::createX86_64TargetInfo; +using llvm::abi::FieldInfo; +using llvm::abi::FunctionInfo; +using llvm::abi::RecordFlags; +using llvm::abi::StructPacking; +using llvm::abi::TargetInfo; +using llvm::abi::TypeBuilder; +using llvm::abi::X86AVXABILevel; + +class X86TargetInfoTest : public ::testing::Test { +protected: + llvm::BumpPtrAllocator Alloc; + TypeBuilder TB; + const ABIType *I8; + const ABIType *I32; + const ABIType *F32; + const ABIType *F64; + const ABIType *Void; + /// An empty class: a record with no fields, one byte wide. + const ABIType *Empty; + /// The same, over-aligned, so it wins the union reduction's alignment + /// comparison. + const ABIType *EmptyOver; + + X86TargetInfoTest() + : TB(Alloc), I8(TB.getIntegerType(8, llvm::Align(1), /*Signed=*/true)), + I32(TB.getIntegerType(32, llvm::Align(4), /*Signed=*/true)), + F32(TB.getFloatType(llvm::APFloat::IEEEsingle(), llvm::Align(4))), + F64(TB.getFloatType(llvm::APFloat::IEEEdouble(), llvm::Align(8))), + Void(TB.getVoidType()), + Empty(TB.getRecordType({}, llvm::TypeSize::getFixed(8), llvm::Align(1), + StructPacking::Default, {}, {}, + RecordFlags::CanPassInRegisters)), + EmptyOver(TB.getRecordType({}, llvm::TypeSize::getFixed(128), + llvm::Align(16), StructPacking::Default, {}, + {}, RecordFlags::CanPassInRegisters)) {} + + std::unique_ptr<TargetInfo> target() const { + return createX86_64TargetInfo(const_cast<TypeBuilder &>(TB), + X86AVXABILevel::None, + /*Has64BitPointers=*/true, ABICompatInfo()); + } + + const ABIType *unionOf(llvm::ArrayRef<FieldInfo> Fields, uint64_t SizeInBits, + llvm::Align Alignment, + RecordFlags Flags = RecordFlags::None) { + return TB.getUnionType(Fields, llvm::TypeSize::getFixed(SizeInBits), + Alignment, StructPacking::Default, + Flags | RecordFlags::CanPassInRegisters); + } + + /// The argument classification the target computes for a single parameter. + const ArgInfo &classifyArg(const ABIType *ArgTy, + std::unique_ptr<FunctionInfo> &FI, + std::unique_ptr<TargetInfo> &TI) { + TI = target(); + FI = FunctionInfo::create(llvm::CallingConv::C, Void, {ArgTy}); + TI->computeInfo(*FI); + return FI->getArgInfo(0).Info; + } +}; + +static void expectDirectInteger(const ArgInfo &Info, unsigned Bits) { + ASSERT_TRUE(Info.isDirect()); + const ABIType *Coerce = Info.getCoerceToType(); + ASSERT_NE(Coerce, nullptr); + const auto *IT = llvm::dyn_cast<llvm::abi::IntegerType>(Coerce); + ASSERT_NE(IT, nullptr); + EXPECT_EQ(IT->getSizeInBits().getFixedValue(), Bits); +} + +static void expectDirectFloat(const ArgInfo &Info, + const llvm::fltSemantics &Sem) { + ASSERT_TRUE(Info.isDirect()); + const ABIType *Coerce = Info.getCoerceToType(); + ASSERT_NE(Coerce, nullptr); + const auto *FT = llvm::dyn_cast<llvm::abi::FloatType>(Coerce); + ASSERT_NE(FT, nullptr); + EXPECT_EQ(FT->getSemantics(), &Sem); +} + +// An empty member supplies no bytes, so the int is the storage the coercion is +// built from and the union coerces to its width. +TEST_F(X86TargetInfoTest, UnionWithEmptyMemberCoercesToDataMember) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *U = + unionOf({FieldInfo(Empty), FieldInfo(I32)}, 32, llvm::Align(4)); + expectDirectInteger(classifyArg(U, FI, TI), 32); +} + +// The empty member's declared alignment outranks the int's, so it wins the +// reduction unless it is skipped. Classic passes this 16-byte union as i32. +TEST_F(X86TargetInfoTest, UnionWithOverAlignedEmptyMemberCoercesToDataMember) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *U = + unionOf({FieldInfo(EmptyOver), FieldInfo(I32)}, 128, llvm::Align(16)); + expectDirectInteger(classifyArg(U, FI, TI), 32); +} + +// The reduction also breaks alignment ties by size, so an array of empty +// records beats a one-byte member without being wider in data. +TEST_F(X86TargetInfoTest, UnionWithArrayOfEmptyMembersCoercesToDataMember) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *ArrEmpty = TB.getArrayType(Empty, /*NumElements=*/2, + /*SizeInBits=*/16); + const ABIType *U = + unionOf({FieldInfo(ArrEmpty), FieldInfo(I8)}, 16, llvm::Align(1)); + expectDirectInteger(classifyArg(U, FI, TI), 8); +} + +// The same at a full eightbyte, where the array of empty records spans the +// union and the coercion still narrows to the one byte of data. +TEST_F(X86TargetInfoTest, UnionWithEightbyteArrayOfEmptyMembersNarrows) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *ArrEmpty = TB.getArrayType(Empty, /*NumElements=*/8, + /*SizeInBits=*/64); + const ABIType *U = + unionOf({FieldInfo(ArrEmpty), FieldInfo(I8)}, 64, llvm::Align(1)); + expectDirectInteger(classifyArg(U, FI, TI), 8); +} + +// A union of nothing but empty members classifies Ignore, the same as an empty +// record does. +TEST_F(X86TargetInfoTest, UnionOfOnlyEmptyMembersIsIgnore) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *U = unionOf({FieldInfo(Empty)}, 8, llvm::Align(1)); + EXPECT_TRUE(classifyArg(U, FI, TI).isIgnore()); +} + +// Skipping the empty member does not force the coercion to be an integer: the +// remaining member still decides the eightbyte's class. +TEST_F(X86TargetInfoTest, UnionWithEmptyMemberKeepsSSEClass) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *U = + unionOf({FieldInfo(Empty), FieldInfo(F64)}, 64, llvm::Align(8)); + expectDirectFloat(classifyArg(U, FI, TI), llvm::APFloat::IEEEdouble()); +} + +// Two floats in one eightbyte still pair into a vector with an empty member +// alongside them. +TEST_F(X86TargetInfoTest, UnionWithEmptyMemberKeepsFloatPair) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *Floats = TB.getRecordType( + {FieldInfo(F32, 0), FieldInfo(F32, 32)}, llvm::TypeSize::getFixed(64), + llvm::Align(4), StructPacking::Default, {}, {}, + RecordFlags::CanPassInRegisters); + const ABIType *U = + unionOf({FieldInfo(Empty), FieldInfo(Floats)}, 64, llvm::Align(4)); + const ArgInfo &Info = classifyArg(U, FI, TI); + ASSERT_TRUE(Info.isDirect()); + const auto *VT = + llvm::dyn_cast_or_null<llvm::abi::VectorType>(Info.getCoerceToType()); + ASSERT_NE(VT, nullptr); + EXPECT_EQ(VT->getNumElements().getFixedValue(), 2u); + const auto *ElemFT = + llvm::dyn_cast<llvm::abi::FloatType>(VT->getElementType()); + ASSERT_NE(ElemFT, nullptr); + EXPECT_EQ(ElemFT->getSemantics(), &llvm::APFloat::IEEEsingle()); +} + +// Where the data member does fill the eightbyte, narrowing must not happen: +// every byte past the first is user data, so the coercion stays i64. +TEST_F(X86TargetInfoTest, UnionWithEmptyMemberDoesNotNarrowOverData) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *Bytes = TB.getArrayType(I8, /*NumElements=*/8, + /*SizeInBits=*/64); + const ABIType *U = + unionOf({FieldInfo(Empty), FieldInfo(Bytes)}, 64, llvm::Align(1)); + expectDirectInteger(classifyArg(U, FI, TI), 64); +} + +// A transparent union is classified as its first field, and skipping empty +// members leaves that alone. The empty-first case is decided by +// useFirstFieldIfTransparentUnion before the reduction runs, so it reaches +// Ignore rather than the reduction's storage-type choice. +TEST_F(X86TargetInfoTest, TransparentUnionTakesFirstField) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + const ABIType *DataFirst = + unionOf({FieldInfo(I32), FieldInfo(F32)}, 32, llvm::Align(4), + RecordFlags::IsTransparent); + expectDirectInteger(classifyArg(DataFirst, FI, TI), 32); + + const ABIType *EmptyFirst = + unionOf({FieldInfo(Empty), FieldInfo(I8)}, 8, llvm::Align(1), + RecordFlags::IsTransparent); + EXPECT_TRUE(classifyArg(EmptyFirst, FI, TI).isIgnore()); +} + +// An unnamed zero-width bit-field is skipped as it was before, so a union of +// nothing else still has no storage type to reduce to. +TEST_F(X86TargetInfoTest, UnionOfZeroWidthBitFieldIsIgnore) { + std::unique_ptr<FunctionInfo> FI; + std::unique_ptr<TargetInfo> TI; + FieldInfo ZeroWidth(I32, 0, /*IsBitField=*/true, /*BitFieldWidth=*/0, + /*IsUnnamedBitField=*/true); + const ABIType *U = unionOf({ZeroWidth}, 8, llvm::Align(1)); + EXPECT_TRUE(classifyArg(U, FI, TI).isIgnore()); +} + +} // namespace _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
