[clang][CodeGen] sret args should always point to the alloca AS, so use that
#114062
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1672,10 +1672,11 @@ CodeGenTypes::GetFunctionType(const CGFunctionInfo &FI) { | |
|
|
||
| // Add type for sret argument. | ||
| if (IRFunctionArgs.hasSRetArg()) { | ||
| auto AddressSpace = CGM.getTarget().getIndirectArgAddressSpace(); | ||
|
||
| if (!AddressSpace) | ||
| AddressSpace = getDataLayout().getAllocaAddrSpace(); | ||
| ArgTypes[IRFunctionArgs.getSRetArgNo()] = | ||
| llvm::PointerType::get(getLLVMContext(), *AddressSpace); | ||
| } | ||
|
|
||
| // Add type for inalloca argument. | ||
|
|
@@ -5145,7 +5146,6 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| // If the call returns a temporary with struct return, create a temporary | ||
| // alloca to hold the result, unless one is given to us. | ||
| Address SRetPtr = Address::invalid(); | ||
| llvm::Value *UnusedReturnSizePtr = nullptr; | ||
| if (RetAI.isIndirect() || RetAI.isInAlloca() || RetAI.isCoerceAndExpand()) { | ||
| // For virtual function pointer thunks and musttail calls, we must always | ||
|
|
@@ -5159,16 +5159,19 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| } else if (!ReturnValue.isNull()) { | ||
| SRetPtr = ReturnValue.getAddress(); | ||
| } else { | ||
| SRetPtr = CreateMemTempWithoutCast(RetTy, "tmp"); | ||
| if (HaveInsertPoint() && ReturnValue.isUnused()) { | ||
| llvm::TypeSize size = | ||
| CGM.getDataLayout().getTypeAllocSize(ConvertTypeForMem(RetTy)); | ||
| UnusedReturnSizePtr = EmitLifetimeStart(size, SRetPtr.getBasePointer()); | ||
| } | ||
| } | ||
| if (IRFunctionArgs.hasSRetArg()) { | ||
| // If the caller allocated the return slot, it is possible that the | ||
AlexVlx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // alloca was AS casted to the default as, so we ensure the cast is | ||
| // stripped before binding to the sret arg, which is in the allocaAS. | ||
|
||
| IRCallArgs[IRFunctionArgs.getSRetArgNo()] = | ||
| getAsNaturalPointerTo(SRetPtr, RetTy)->stripPointerCasts(); | ||
| } else if (RetAI.isInAlloca()) { | ||
| Address Addr = | ||
| Builder.CreateStructGEP(ArgMemory, RetAI.getInAllocaFieldIndex()); | ||
|
|
@@ -5390,11 +5393,19 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| V->getType()->isIntegerTy()) | ||
| V = Builder.CreateZExt(V, ArgInfo.getCoerceToType()); | ||
|
|
||
| // If the argument doesn't match, we are either trying to pass an | ||
| // alloca-ed sret argument directly, and the alloca AS does not match | ||
| // the default AS, case in which we AS cast it, or we have a trivial | ||
| // type mismatch, and thus perform a bitcast to coerce it. | ||
|
||
| if (FirstIRArg < IRFuncTy->getNumParams() && | ||
| V->getType() != IRFuncTy->getParamType(FirstIRArg)) { | ||
| auto IRTy = IRFuncTy->getParamType(FirstIRArg); | ||
| auto MaybeSRetArg = dyn_cast_or_null<llvm::Argument>(V); | ||
| if (MaybeSRetArg && MaybeSRetArg->hasStructRetAttr()) | ||
| V = Builder.CreateAddrSpaceCast(V, IRTy); | ||
|
||
| else | ||
| V = Builder.CreateBitCast(V, IRTy); | ||
|
||
| } | ||
|
|
||
| if (ArgHasMaybeUndefAttr) | ||
| V = Builder.CreateFreeze(V); | ||
|
|
@@ -5740,7 +5751,7 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| // pop this cleanup later on. Being eager about this is OK, since this | ||
| // temporary is 'invisible' outside of the callee. | ||
| if (UnusedReturnSizePtr) | ||
| pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker, SRetPtr, | ||
| UnusedReturnSizePtr); | ||
|
|
||
| llvm::BasicBlock *InvokeDest = CannotThrow ? nullptr : getInvokeDest(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,34 @@ | ||
| // RUN: %clang_cc1 %s -Wno-strict-prototypes -emit-llvm -o - | FileCheck %s | ||
| // RUN: %clang_cc1 %s -Wno-strict-prototypes -triple amdgcn-amd-amdhsa -emit-llvm -o - | FileCheck --check-prefix=NONZEROALLOCAAS %s | ||
|
|
||
| struct abc { | ||
| long a; | ||
| long b; | ||
| long c; | ||
| long d; | ||
| long e; | ||
| long f; | ||
| long g; | ||
| long h; | ||
| long i; | ||
| long j; | ||
| }; | ||
|
|
||
| struct abc foo1(void); | ||
| // CHECK-DAG: declare {{.*}} @foo1(ptr dead_on_unwind writable sret(%struct.abc) | ||
| // NONZEROALLOCAAS-DAG: declare {{.*}} @foo1(ptr addrspace(5) dead_on_unwind writable sret(%struct.abc) | ||
| struct abc foo2(); | ||
| // CHECK-DAG: declare {{.*}} @foo2(ptr dead_on_unwind writable sret(%struct.abc) | ||
| // NONZEROALLOCAAS-DAG: declare {{.*}} @foo2(ptr addrspace(5) dead_on_unwind writable sret(%struct.abc) | ||
| struct abc foo3(void){} | ||
| // CHECK-DAG: define {{.*}} @foo3(ptr dead_on_unwind noalias writable sret(%struct.abc) | ||
| // NONZEROALLOCAAS-DAG: define {{.*}} @foo3(ptr addrspace(5) dead_on_unwind noalias writable sret(%struct.abc) | ||
|
|
||
| void bar(void) { | ||
| struct abc dummy1 = foo1(); | ||
| // CHECK-DAG: call {{.*}} @foo1(ptr dead_on_unwind writable sret(%struct.abc) | ||
| // NONZEROALLOCAAS-DAG: call {{.*}} @foo1(ptr addrspace(5) dead_on_unwind writable sret(%struct.abc) | ||
| struct abc dummy2 = foo2(); | ||
| // CHECK-DAG: call {{.*}} @foo2(ptr dead_on_unwind writable sret(%struct.abc) | ||
| // NONZEROALLOCAAS-DAG: call {{.*}} @foo2(ptr addrspace(5) dead_on_unwind writable sret(%struct.abc) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this to be a
LangAS, since that's what our address-space conversion lowerings are generally expressed in terms of. This also has the advantage of avoiding a lot of heartache with implicit conversions aroundABIInfo::getIndirect, sinceLangASis a scoped enum. AndLangAS::Defaultis a much more reasonable default argument for things likeABIArgInfo::getIndirectthan IR addrspace 0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also shouldn't be optional. There always must be a definitive IR address space.
Also I'm not sure I follow why this is still necessary if you've modified getIndirect to carry the address space. RetAI should have this info now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, I forgot to delete this. RE:
unsignedvsLangAS, I used the former for symmetry with other interfaces. I agree with @rjmccall thatLangASwould be safer / would make more sense, howevergetIndirectAliasedalready uses numeric vs typed.