[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
https://github.com/efriedma-quic edited https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
https://github.com/efriedma-quic edited https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
github-actions[bot] wrote: :warning: C/C++ code formatter, clang-format found issues in your code. :warning: You can test this locally with the following command: ``bash git-clang-format --diff 0012b1ea84e671a9e0c9f7f2d1564315ed9cbcca 19f3b677d92ed88b825b455d738055da05f91e0d -- clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CGExprAgg.cpp clang/lib/CodeGen/CodeGenFunction.h clang/test/CodeGen/arm-mve-intrinsics/vld24.c clang/test/CodeGen/arm-vfp16-arguments2.cpp clang/test/CodeGenCXX/address-space-cast-coerce.cpp clang/test/CodeGenCXX/cxx2a-consteval.cpp clang/test/CodeGenCXX/trivial_abi.cpp `` View the diff from clang-format here. ``diff diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index db6feb4bf1..262aa30547 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -1355,7 +1355,8 @@ void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, // FIXME: This isn't really that useful with opaque types, but it impacts a // lot of regression tests. if (SrcTy != Dst.getElementType()) { -if (llvm::StructType *DstSTy = dyn_cast(Dst.getElementType())) { +if (llvm::StructType *DstSTy = +dyn_cast(Dst.getElementType())) { assert(!SrcSize.isScalable()); Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, SrcSize.getFixedValue(), *this); @@ -1368,7 +1369,8 @@ void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, // If the value is supposed to be a pointer, convert it before storing it. Src = CoerceIntOrPtrToIntOrPtr(Src, Dst.getElementType(), *this); Builder.CreateStore(Src, Dst, DstIsVolatile); -} else if (llvm::StructType *STy = dyn_cast(Src->getType())) { +} else if (llvm::StructType *STy = + dyn_cast(Src->getType())) { // Prefer scalar stores to first-class aggregate stores. Dst = Dst.withElementType(SrcTy); for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) { `` https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -177,7 +179,12 @@ kernel void KernelTwoMember(struct StructTwoMember u) { // AMDGCN-LABEL: define{{.*}} amdgpu_kernel void @KernelLargeTwoMember // AMDGCN-SAME: (%struct.LargeStructTwoMember %[[u_coerce:.*]]) // AMDGCN: %[[u:.*]] = alloca %struct.LargeStructTwoMember, align 8, addrspace(5) -// AMDGCN: store %struct.LargeStructTwoMember %[[u_coerce]], ptr addrspace(5) %[[u]] +// AMDGCN: %[[U_PTR0:.*]] = getelementptr inbounds %struct.LargeStructTwoMember, ptr addrspace(5) %[[u]], i32 0, i32 0 efriedma-quic wrote: Unifying the codepaths makes FCA promotion happen more often. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -46,9 +46,9 @@ int mane() { char1 f1{1}; char1 f2{1}; -// CHECK: [[TMP:%.+]] = alloca i16 efriedma-quic wrote: The revised version of casting integers is a bit more aggressive; it's hard to make it precisely match the old code while still preserving the correct semantics. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -44,20 +44,20 @@ struct S1 f1(struct S1 s1) { return s1; } // CHECK-SOFT: define{{.*}} void @_Z2f22S2(ptr dead_on_unwind noalias nocapture writable writeonly sret(%struct.S2) align 8 %agg.result, [4 x i32] %s2.coerce) // CHECK-HARD: define{{.*}} arm_aapcs_vfpcc [2 x <2 x i32>] @_Z2f22S2([2 x <2 x i32>] returned %s2.coerce) -// CHECK-FULL: define{{.*}} arm_aapcs_vfpcc %struct.S2 @_Z2f22S2(%struct.S2 returned %s2.coerce) +// CHECK-FULL: define{{.*}} arm_aapcs_vfpcc %struct.S2 @_Z2f22S2(%struct.S2 %s2.coerce) efriedma-quic wrote: This is also the instcombine issue. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -48,10 +48,13 @@ uint8x16x4_t test_vld4q_u8(const uint8_t *addr) // CHECK-LABEL: @test_vst2q_u32( // CHECK-NEXT: entry: -// CHECK-NEXT:[[VALUE_COERCE_FCA_0_0_EXTRACT:%.*]] = extractvalue [[STRUCT_UINT32X4X2_T:%.*]] [[VALUE_COERCE:%.*]], 0, 0 -// CHECK-NEXT:[[VALUE_COERCE_FCA_0_1_EXTRACT:%.*]] = extractvalue [[STRUCT_UINT32X4X2_T]] [[VALUE_COERCE]], 0, 1 -// CHECK-NEXT:call void @llvm.arm.mve.vst2q.p0.v4i32(ptr [[ADDR:%.*]], <4 x i32> [[VALUE_COERCE_FCA_0_0_EXTRACT]], <4 x i32> [[VALUE_COERCE_FCA_0_1_EXTRACT]], i32 0) -// CHECK-NEXT:call void @llvm.arm.mve.vst2q.p0.v4i32(ptr [[ADDR]], <4 x i32> [[VALUE_COERCE_FCA_0_0_EXTRACT]], <4 x i32> [[VALUE_COERCE_FCA_0_1_EXTRACT]], i32 1) +// CHECK-NEXT:[[TMP0:%.*]] = extractvalue [[STRUCT_UINT32X4X2_T:%.*]] [[VALUE_COERCE:%.*]], 0 efriedma-quic wrote: Apparently I've stumbled over some limitation of instcombine. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
https://github.com/efriedma-quic updated https://github.com/llvm/llvm-project/pull/93115 >From 816ceb271c28ff6c4dc05485000deec087674e37 Mon Sep 17 00:00:00 2001 From: Eli Friedman Date: Thu, 23 May 2024 18:38:04 -0700 Subject: [PATCH] Fix codegen of consteval functions returning an empty class, and related issues If a class is empty, don't store it to memory: the store might overwrite useful data. Similarly, if a class has tail padding that might overlap other fields, don't store the tail padding to memory. The problem here turned out a bit more general than I initially thought: basically all uses of EmitAggregateStore were broken. Call lowering had a method that did mostly the right thing, though: CreateCoercedStore. Adapt CreateCoercedStore so it always does the conservatively right thing, and use it for both calls and ConstantExpr. Also, along the way, fix the "overlap" bit in AggValueSlot: the bit was set incorrectly for empty classes in some cases. Fixes #93040. --- clang/lib/CodeGen/CGCall.cpp | 140 -- clang/lib/CodeGen/CGExprAgg.cpp | 23 +-- clang/lib/CodeGen/CodeGenFunction.h | 3 +- clang/test/CodeGen/arm-mve-intrinsics/vld24.c | 43 -- clang/test/CodeGen/arm-vfp16-arguments2.cpp | 10 +- .../amdgpu-kernel-arg-pointer-type.cu | 11 +- clang/test/CodeGenCUDA/builtins-amdgcn.cu | 121 +++ .../CodeGenCXX/address-space-cast-coerce.cpp | 6 +- clang/test/CodeGenCXX/cxx2a-consteval.cpp | 24 ++- clang/test/CodeGenCXX/trivial_abi.cpp | 20 +++ clang/test/CodeGenHIP/dpp-const-fold.hip | 8 +- .../CodeGenOpenCL/addr-space-struct-arg.cl| 11 +- .../amdgpu-abi-struct-arg-byref.cl| 42 -- 13 files changed, 255 insertions(+), 207 deletions(-) diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index 97449a5e51e73..262aa3054754c 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -1336,75 +1336,56 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty, return CGF.Builder.CreateLoad(Tmp); } -// Function to store a first-class aggregate into memory. We prefer to -// store the elements rather than the aggregate to be more friendly to -// fast-isel. -// FIXME: Do we need to recurse here? -void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest, - bool DestIsVolatile) { - // Prefer scalar stores to first-class aggregate stores. - if (llvm::StructType *STy = dyn_cast(Val->getType())) { -for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) { - Address EltPtr = Builder.CreateStructGEP(Dest, i); - llvm::Value *Elt = Builder.CreateExtractValue(Val, i); - Builder.CreateStore(Elt, EltPtr, DestIsVolatile); -} - } else { -Builder.CreateStore(Val, Dest, DestIsVolatile); - } -} - /// CreateCoercedStore - Create a store to \arg DstPtr from \arg Src, /// where the source and destination may have different types. The /// destination is known to be aligned to \arg DstAlign bytes. /// /// This safely handles the case when the src type is larger than the /// destination type; the upper bits of the src will be lost. -static void CreateCoercedStore(llvm::Value *Src, - Address Dst, - bool DstIsVolatile, - CodeGenFunction &CGF) { - llvm::Type *SrcTy = Src->getType(); - llvm::Type *DstTy = Dst.getElementType(); - if (SrcTy == DstTy) { -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; - } - - llvm::TypeSize SrcSize = CGF.CGM.getDataLayout().getTypeAllocSize(SrcTy); - - if (llvm::StructType *DstSTy = dyn_cast(DstTy)) { -Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, - SrcSize.getFixedValue(), CGF); -DstTy = Dst.getElementType(); - } - - llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy); - llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy); - if (SrcPtrTy && DstPtrTy && - SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) { -Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); +void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, + llvm::TypeSize DstSize, + bool DstIsVolatile) { + if (!DstSize) return; - } - // If the source and destination are integer or pointer types, just do an - // extension or truncation to the desired type. - if ((isa(SrcTy) || isa(SrcTy)) && - (isa(DstTy) || isa(DstTy))) { -Src = CoerceIntOrPtrToIntOrPtr(Src, DstTy, CGF); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; + llvm::Type *SrcTy = Src->getType(); + llvm::TypeSize SrcSize = CGM.getDataLayout().getTypeAllocSize(SrcTy); + + // GEP into structs to try to
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
efriedma-quic wrote: (I'd like a re-review of the latest version: I made significant revisions to address the tail-padding issues.) https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
https://github.com/zygoloid approved this pull request. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
https://github.com/zygoloid edited https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -1336,75 +1336,56 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty, return CGF.Builder.CreateLoad(Tmp); } -// Function to store a first-class aggregate into memory. We prefer to -// store the elements rather than the aggregate to be more friendly to -// fast-isel. -// FIXME: Do we need to recurse here? -void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest, - bool DestIsVolatile) { - // Prefer scalar stores to first-class aggregate stores. - if (llvm::StructType *STy = dyn_cast(Val->getType())) { -for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) { - Address EltPtr = Builder.CreateStructGEP(Dest, i); - llvm::Value *Elt = Builder.CreateExtractValue(Val, i); - Builder.CreateStore(Elt, EltPtr, DestIsVolatile); -} - } else { -Builder.CreateStore(Val, Dest, DestIsVolatile); - } -} - /// CreateCoercedStore - Create a store to \arg DstPtr from \arg Src, /// where the source and destination may have different types. The /// destination is known to be aligned to \arg DstAlign bytes. /// /// This safely handles the case when the src type is larger than the /// destination type; the upper bits of the src will be lost. -static void CreateCoercedStore(llvm::Value *Src, - Address Dst, - bool DstIsVolatile, - CodeGenFunction &CGF) { - llvm::Type *SrcTy = Src->getType(); - llvm::Type *DstTy = Dst.getElementType(); - if (SrcTy == DstTy) { -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; - } - - llvm::TypeSize SrcSize = CGF.CGM.getDataLayout().getTypeAllocSize(SrcTy); - - if (llvm::StructType *DstSTy = dyn_cast(DstTy)) { -Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, - SrcSize.getFixedValue(), CGF); -DstTy = Dst.getElementType(); - } - - llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy); - llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy); - if (SrcPtrTy && DstPtrTy && - SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) { -Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); +void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, + llvm::TypeSize DstSize, + bool DstIsVolatile) { + if (!DstSize) return; - } - // If the source and destination are integer or pointer types, just do an - // extension or truncation to the desired type. - if ((isa(SrcTy) || isa(SrcTy)) && - (isa(DstTy) || isa(DstTy))) { -Src = CoerceIntOrPtrToIntOrPtr(Src, DstTy, CGF); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; + llvm::Type *SrcTy = Src->getType(); + llvm::TypeSize SrcSize = CGM.getDataLayout().getTypeAllocSize(SrcTy); + + // GEP into structs to try to make types match. + // FIXME: This isn't really that useful with opaque types, but it impacts a + // lot of regression tests. + if (SrcTy != Dst.getElementType()) { +if (llvm::StructType *DstSTy = +dyn_cast(Dst.getElementType())) { + assert(!SrcSize.isScalable()); + Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, + SrcSize.getFixedValue(), *this); +} } - llvm::TypeSize DstSize = CGF.CGM.getDataLayout().getTypeAllocSize(DstTy); - - // If store is legal, just bitcast the src pointer. - if (isa(SrcTy) || - isa(DstTy) || - SrcSize.getFixedValue() <= DstSize.getFixedValue()) { -Dst = Dst.withElementType(SrcTy); -CGF.EmitAggregateStore(Src, Dst, DstIsVolatile); + if (SrcSize.isScalable() || SrcSize <= DstSize) { +if (SrcTy->isIntegerTy() && Dst.getElementType()->isPointerTy() && +SrcSize == CGM.getDataLayout().getTypeAllocSize(Dst.getElementType())) { + // If the value is supposed to be a pointer, convert it before storing it. + Src = CoerceIntOrPtrToIntOrPtr(Src, Dst.getElementType(), *this); zygoloid wrote: It looks like we used to create either an `inttoptr` or `ptrtoint` here and now we only handle the `inttoptr` case. Is that OK? https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -4772,7 +4772,8 @@ class CodeGenFunction : public CodeGenTypeCache { /// Build all the stores needed to initialize an aggregate at Dest with the /// value Val. zygoloid wrote: This comment looks out of date. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -1336,75 +1336,56 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty, return CGF.Builder.CreateLoad(Tmp); } -// Function to store a first-class aggregate into memory. We prefer to -// store the elements rather than the aggregate to be more friendly to -// fast-isel. -// FIXME: Do we need to recurse here? -void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest, - bool DestIsVolatile) { - // Prefer scalar stores to first-class aggregate stores. - if (llvm::StructType *STy = dyn_cast(Val->getType())) { -for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) { - Address EltPtr = Builder.CreateStructGEP(Dest, i); - llvm::Value *Elt = Builder.CreateExtractValue(Val, i); - Builder.CreateStore(Elt, EltPtr, DestIsVolatile); -} - } else { -Builder.CreateStore(Val, Dest, DestIsVolatile); - } -} - /// CreateCoercedStore - Create a store to \arg DstPtr from \arg Src, /// where the source and destination may have different types. The /// destination is known to be aligned to \arg DstAlign bytes. /// /// This safely handles the case when the src type is larger than the /// destination type; the upper bits of the src will be lost. -static void CreateCoercedStore(llvm::Value *Src, - Address Dst, - bool DstIsVolatile, - CodeGenFunction &CGF) { - llvm::Type *SrcTy = Src->getType(); - llvm::Type *DstTy = Dst.getElementType(); - if (SrcTy == DstTy) { -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; - } - - llvm::TypeSize SrcSize = CGF.CGM.getDataLayout().getTypeAllocSize(SrcTy); - - if (llvm::StructType *DstSTy = dyn_cast(DstTy)) { -Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, - SrcSize.getFixedValue(), CGF); -DstTy = Dst.getElementType(); - } - - llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy); - llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy); - if (SrcPtrTy && DstPtrTy && - SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) { -Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); +void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, + llvm::TypeSize DstSize, + bool DstIsVolatile) { + if (!DstSize) return; - } - // If the source and destination are integer or pointer types, just do an - // extension or truncation to the desired type. - if ((isa(SrcTy) || isa(SrcTy)) && - (isa(DstTy) || isa(DstTy))) { -Src = CoerceIntOrPtrToIntOrPtr(Src, DstTy, CGF); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; + llvm::Type *SrcTy = Src->getType(); + llvm::TypeSize SrcSize = CGM.getDataLayout().getTypeAllocSize(SrcTy); + + // GEP into structs to try to make types match. + // FIXME: This isn't really that useful with opaque types, but it impacts a + // lot of regression tests. + if (SrcTy != Dst.getElementType()) { +if (llvm::StructType *DstSTy = +dyn_cast(Dst.getElementType())) { + assert(!SrcSize.isScalable()); + Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, + SrcSize.getFixedValue(), *this); +} } - llvm::TypeSize DstSize = CGF.CGM.getDataLayout().getTypeAllocSize(DstTy); - - // If store is legal, just bitcast the src pointer. - if (isa(SrcTy) || - isa(DstTy) || - SrcSize.getFixedValue() <= DstSize.getFixedValue()) { -Dst = Dst.withElementType(SrcTy); -CGF.EmitAggregateStore(Src, Dst, DstIsVolatile); + if (SrcSize.isScalable() || SrcSize <= DstSize) { +if (SrcTy->isIntegerTy() && Dst.getElementType()->isPointerTy() && +SrcSize == CGM.getDataLayout().getTypeAllocSize(Dst.getElementType())) { + // If the value is supposed to be a pointer, convert it before storing it. + Src = CoerceIntOrPtrToIntOrPtr(Src, Dst.getElementType(), *this); efriedma-quic wrote: inttoptr comes from constructs like a struct containing a pointer, which the calling convention code sometimes chooses to just coerce to an integer instead of trying to compute a prettier type. The other direction would be very unusual: it would mean we somehow coerced some non-pointer value to a pointer. And this isn't really a correctness thing, anyway; type-punning in memory is basically the same as doing it in a register. This just makes the intended types more clear for optimizations. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -1336,75 +1336,56 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty, return CGF.Builder.CreateLoad(Tmp); } -// Function to store a first-class aggregate into memory. We prefer to -// store the elements rather than the aggregate to be more friendly to -// fast-isel. -// FIXME: Do we need to recurse here? -void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest, - bool DestIsVolatile) { - // Prefer scalar stores to first-class aggregate stores. - if (llvm::StructType *STy = dyn_cast(Val->getType())) { -for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) { - Address EltPtr = Builder.CreateStructGEP(Dest, i); - llvm::Value *Elt = Builder.CreateExtractValue(Val, i); - Builder.CreateStore(Elt, EltPtr, DestIsVolatile); -} - } else { -Builder.CreateStore(Val, Dest, DestIsVolatile); - } -} - /// CreateCoercedStore - Create a store to \arg DstPtr from \arg Src, /// where the source and destination may have different types. The /// destination is known to be aligned to \arg DstAlign bytes. /// /// This safely handles the case when the src type is larger than the /// destination type; the upper bits of the src will be lost. -static void CreateCoercedStore(llvm::Value *Src, - Address Dst, - bool DstIsVolatile, - CodeGenFunction &CGF) { - llvm::Type *SrcTy = Src->getType(); - llvm::Type *DstTy = Dst.getElementType(); - if (SrcTy == DstTy) { -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; - } - - llvm::TypeSize SrcSize = CGF.CGM.getDataLayout().getTypeAllocSize(SrcTy); - - if (llvm::StructType *DstSTy = dyn_cast(DstTy)) { -Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, - SrcSize.getFixedValue(), CGF); -DstTy = Dst.getElementType(); - } - - llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy); - llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy); - if (SrcPtrTy && DstPtrTy && - SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) { -Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); +void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, + llvm::TypeSize DstSize, + bool DstIsVolatile) { + if (!DstSize) return; - } - // If the source and destination are integer or pointer types, just do an - // extension or truncation to the desired type. - if ((isa(SrcTy) || isa(SrcTy)) && - (isa(DstTy) || isa(DstTy))) { -Src = CoerceIntOrPtrToIntOrPtr(Src, DstTy, CGF); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; + llvm::Type *SrcTy = Src->getType(); + llvm::TypeSize SrcSize = CGM.getDataLayout().getTypeAllocSize(SrcTy); + + // GEP into structs to try to make types match. + // FIXME: This isn't really that useful with opaque types, but it impacts a + // lot of regression tests. + if (SrcTy != Dst.getElementType()) { +if (llvm::StructType *DstSTy = +dyn_cast(Dst.getElementType())) { + assert(!SrcSize.isScalable()); + Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, + SrcSize.getFixedValue(), *this); +} } - llvm::TypeSize DstSize = CGF.CGM.getDataLayout().getTypeAllocSize(DstTy); - - // If store is legal, just bitcast the src pointer. - if (isa(SrcTy) || - isa(DstTy) || - SrcSize.getFixedValue() <= DstSize.getFixedValue()) { -Dst = Dst.withElementType(SrcTy); -CGF.EmitAggregateStore(Src, Dst, DstIsVolatile); + if (SrcSize.isScalable() || SrcSize <= DstSize) { +if (SrcTy->isIntegerTy() && Dst.getElementType()->isPointerTy() && +SrcSize == CGM.getDataLayout().getTypeAllocSize(Dst.getElementType())) { + // If the value is supposed to be a pointer, convert it before storing it. + Src = CoerceIntOrPtrToIntOrPtr(Src, Dst.getElementType(), *this); efriedma-quic wrote: The original code implicitly handled int->int, int->ptr, and ptr->int. But ptr->int doesn't come up in practice https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
@@ -1336,75 +1336,56 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty, return CGF.Builder.CreateLoad(Tmp); } -// Function to store a first-class aggregate into memory. We prefer to -// store the elements rather than the aggregate to be more friendly to -// fast-isel. -// FIXME: Do we need to recurse here? -void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest, - bool DestIsVolatile) { - // Prefer scalar stores to first-class aggregate stores. - if (llvm::StructType *STy = dyn_cast(Val->getType())) { -for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) { - Address EltPtr = Builder.CreateStructGEP(Dest, i); - llvm::Value *Elt = Builder.CreateExtractValue(Val, i); - Builder.CreateStore(Elt, EltPtr, DestIsVolatile); -} - } else { -Builder.CreateStore(Val, Dest, DestIsVolatile); - } -} - /// CreateCoercedStore - Create a store to \arg DstPtr from \arg Src, /// where the source and destination may have different types. The /// destination is known to be aligned to \arg DstAlign bytes. /// /// This safely handles the case when the src type is larger than the /// destination type; the upper bits of the src will be lost. -static void CreateCoercedStore(llvm::Value *Src, - Address Dst, - bool DstIsVolatile, - CodeGenFunction &CGF) { - llvm::Type *SrcTy = Src->getType(); - llvm::Type *DstTy = Dst.getElementType(); - if (SrcTy == DstTy) { -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; - } - - llvm::TypeSize SrcSize = CGF.CGM.getDataLayout().getTypeAllocSize(SrcTy); - - if (llvm::StructType *DstSTy = dyn_cast(DstTy)) { -Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, - SrcSize.getFixedValue(), CGF); -DstTy = Dst.getElementType(); - } - - llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy); - llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy); - if (SrcPtrTy && DstPtrTy && - SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) { -Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); +void CodeGenFunction::CreateCoercedStore(llvm::Value *Src, Address Dst, + llvm::TypeSize DstSize, + bool DstIsVolatile) { + if (!DstSize) return; - } - // If the source and destination are integer or pointer types, just do an - // extension or truncation to the desired type. - if ((isa(SrcTy) || isa(SrcTy)) && - (isa(DstTy) || isa(DstTy))) { -Src = CoerceIntOrPtrToIntOrPtr(Src, DstTy, CGF); -CGF.Builder.CreateStore(Src, Dst, DstIsVolatile); -return; + llvm::Type *SrcTy = Src->getType(); + llvm::TypeSize SrcSize = CGM.getDataLayout().getTypeAllocSize(SrcTy); + + // GEP into structs to try to make types match. + // FIXME: This isn't really that useful with opaque types, but it impacts a + // lot of regression tests. + if (SrcTy != Dst.getElementType()) { +if (llvm::StructType *DstSTy = +dyn_cast(Dst.getElementType())) { + assert(!SrcSize.isScalable()); + Dst = EnterStructPointerForCoercedAccess(Dst, DstSTy, + SrcSize.getFixedValue(), *this); +} } - llvm::TypeSize DstSize = CGF.CGM.getDataLayout().getTypeAllocSize(DstTy); - - // If store is legal, just bitcast the src pointer. - if (isa(SrcTy) || - isa(DstTy) || - SrcSize.getFixedValue() <= DstSize.getFixedValue()) { -Dst = Dst.withElementType(SrcTy); -CGF.EmitAggregateStore(Src, Dst, DstIsVolatile); + if (SrcSize.isScalable() || SrcSize <= DstSize) { +if (SrcTy->isIntegerTy() && Dst.getElementType()->isPointerTy() && +SrcSize == CGM.getDataLayout().getTypeAllocSize(Dst.getElementType())) { + // If the value is supposed to be a pointer, convert it before storing it. + Src = CoerceIntOrPtrToIntOrPtr(Src, Dst.getElementType(), *this); zygoloid wrote: Yeah, it definitely seems like it'd be a pretty big surprise to need this. OK. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
https://github.com/efriedma-quic closed https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
RSilicon wrote: Is this worth back porting as it is a bugfix over code gen? https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
AaronBallman wrote: > Is this worth back porting as it is a bugfix over code gen? IMO, it's worth considering, but if we want to go down this route, I think we need to do so relatively quickly -- we have about two weeks until rc3, and given the size of this change, I'm not certain we should try landing it any later than rc3 just due to risk. https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix codegen of consteval functions returning an empty class, and related issues (PR #93115)
efriedma-quic wrote: /cherry-pick 1762e01cca0186f1862db561cfd9019164b8c654 https://github.com/llvm/llvm-project/pull/93115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits