-
Notifications
You must be signed in to change notification settings - Fork 18.1k
[Clang] Forward incoming Indirect parameters across musttail calls #199351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 22 commits
cf30821
a1c3fd3
11cdbbf
4359083
9b1aa8b
99dfa79
88607e8
2dd4ce8
a27213a
d12920e
7231b68
f008bf5
1248313
40a7edf
6964a03
3a58836
6f56dba
cb7efcb
3a8db70
2a770e5
5be3d32
9905d11
8c5c3be
beda9ba
d6fe44f
c3ba863
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5253,6 +5253,42 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E, | |
| } | ||
| } | ||
|
|
||
| // C++ analog of the CK_LValueToRValue case above: a trivial copy/move | ||
| // constructor from a variable forwards the source LValue instead of | ||
| // materializing an agg.tmp. EmitCall makes the real copy at the | ||
| // Indirect/byval boundary. Under musttail this also keeps the value in | ||
| // storage that survives the tail call. Types whose copy is not a plain | ||
| // memcpy in EmitAggregateCopy are excluded, since forwarding would bypass | ||
| // that special copy: CUDA surface/texture (lowered to a handle) and, under | ||
| // ObjC GC, records with object members (need a write barrier). | ||
| if (HasAggregateEvalKind && type->isRecordType() && | ||
| type.isTriviallyCopyableType(getContext()) && | ||
| !type->isCUDADeviceBuiltinSurfaceType() && | ||
| !type->isCUDADeviceBuiltinTextureType() && | ||
| !(getLangOpts().getGC() != LangOptions::NonGC && | ||
| type->getAsRecordDecl()->hasObjectMember())) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little worried this is going to fall out of date... does this list just come from digging through CodeGenFunction::EmitAggregateCopy? Is there anywhere else that uses a similar check?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I copied EmitAggregateCopy's special cases, and I found no existing shared check for "this copy is a plain memcpy". I will factorize this, the risks of drift is concerning. A "cleaner" alternative would be to route the argument copy through EmitAggregateCopy itself so no list is needed, but might be more complex ? |
||
| if (const auto *CCE = dyn_cast<CXXConstructExpr>(E)) { | ||
| const CXXConstructorDecl *Ctor = CCE->getConstructor(); | ||
| if (Ctor->isCopyOrMoveConstructor() && Ctor->isTrivial() && | ||
| CCE->getNumArgs() == 1) { | ||
| const Expr *Source = CCE->getArg(0)->IgnoreParenImpCasts(); | ||
|
xroche marked this conversation as resolved.
Outdated
|
||
| // Same-type guard: a stripped derived-to-base cast would forward a | ||
| // derived lvalue into a base-typed slot and slice at a wrong offset. | ||
| // Exclude hlsl_constant sources, as the CK_LValueToRValue path does. | ||
| if (const auto *DRE = dyn_cast<DeclRefExpr>(Source); | ||
| DRE && isa<VarDecl>(DRE->getDecl()) && | ||
| Source->getType().getAddressSpace() != LangAS::hlsl_constant && | ||
| getContext().hasSameUnqualifiedType(Source->getType(), type)) { | ||
| LValue L = EmitLValue(DRE); | ||
| if (L.isSimple()) { | ||
| args.addUncopiedAggregate(L, type); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| args.add(EmitAnyExprToTemp(E), type); | ||
| } | ||
|
|
||
|
|
@@ -5562,6 +5598,17 @@ static unsigned getMaxVectorWidth(const llvm::Type *Ty) { | |
| return MaxVectorWidth; | ||
| } | ||
|
|
||
| /// Peel one AddrSpaceCastInst from \p SrcPtr. EmitParmDecl wraps incoming | ||
| /// Indirect params via address-space cast on NVPTX/AMDGPU/SPIR, so peeling | ||
| /// exposes the underlying llvm::Argument when the source IS a forwarded | ||
| /// incoming parameter. Loads are NOT unwrapped: a load through a local | ||
| /// alloca means the source is a local. | ||
| static llvm::Value *peelAddrSpaceCast(llvm::Value *SrcPtr) { | ||
| if (auto *ASC = llvm::dyn_cast<llvm::AddrSpaceCastInst>(SrcPtr)) | ||
| return ASC->getOperand(0); | ||
| return SrcPtr; | ||
| } | ||
|
|
||
| RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | ||
| const CGCallee &Callee, | ||
| ReturnValueSlot ReturnValue, | ||
|
|
@@ -5685,6 +5732,17 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| // markers that need to be ended right after the call. | ||
| SmallVector<CallLifetimeEnd, 2> CallLifetimeEndAfterCall; | ||
|
|
||
| // Deferred Phase-2 writes for musttail Indirect args. Splitting reads | ||
| // (in the per-arg loop) from writes (after the loop) lets permutations | ||
| // like C(b, a) land correctly: all sources are captured into scratches | ||
| // before any incoming-param destination is overwritten. | ||
| struct MustTailIndirectCopy { | ||
| LValue Scratch; | ||
| LValue Dst; | ||
| QualType Ty; | ||
| }; | ||
| llvm::SmallVector<MustTailIndirectCopy, 4> MustTailIndirectCopies; | ||
|
|
||
| // Translate all of the arguments as necessary to match the IR lowering. | ||
| assert(CallInfo.arg_size() == CallArgs.size() && | ||
| "Mismatch between function signature & arguments."); | ||
|
|
@@ -5757,6 +5815,51 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| case ABIArgInfo::Indirect: | ||
| case ABIArgInfo::IndirectAliased: { | ||
| assert(NumIRArgs == 1); | ||
|
|
||
| // Musttail Indirect: route via the matching incoming parameter. | ||
| // Prototype-match (Verifier V5/V6/V7) makes CurFn->arg_begin()+ | ||
| // FirstIRArg a distinct destination for this slot that lives in the | ||
| // caller's caller's frame and survives the tail call. To handle | ||
| // permutations safely, the source value is captured into a scratch | ||
| // alloca here (Phase 1); the write to the incoming-param destination | ||
| // is deferred until after all sources have been read (Phase 2 below). | ||
| // IndirectAliased uses the existing fallback (different source-AS). | ||
| if (IsMustTail && ArgInfo.isIndirect()) { | ||
| llvm::Argument *IncomingArg = CurFn->arg_begin() + FirstIRArg; | ||
| llvm::Value *Dst = IncomingArg; | ||
| Address SrcAddr = Address::invalid(); | ||
| if (I->hasLValue()) | ||
| SrcAddr = I->getKnownLValue().getAddress(); | ||
| else if (I->getKnownRValue().isAggregate()) | ||
| SrcAddr = I->getKnownRValue().getAggregateAddress(); | ||
| if (SrcAddr.isValid()) { | ||
| llvm::Value *Src = peelAddrSpaceCast(SrcAddr.emitRawPointer(*this)); | ||
| if (Src != Dst) { | ||
| CharUnits Align = ArgInfo.getIndirectAlign(); | ||
| QualType Ty = I->Ty; | ||
| llvm::Type *ElemTy = ConvertTypeForMem(Ty); | ||
| RawAddress Scratch = | ||
| CreateMemTempWithoutCast(Ty, Align, "musttail.copy"); | ||
| LValue ScratchLV = MakeAddrLValue(Scratch, Ty); | ||
| LValue SrcLV = MakeAddrLValue(SrcAddr, Ty); | ||
| EmitAggregateCopy(ScratchLV, SrcLV, Ty, | ||
| AggValueSlot::DoesNotOverlap); | ||
| LValue DstLV = MakeAddrLValue(Address(Dst, ElemTy, Align), Ty); | ||
| MustTailIndirectCopies.push_back({ScratchLV, DstLV, Ty}); | ||
| } | ||
| // No freeze: Dst is an incoming parameter pointer, never poison. | ||
| IRCallArgs[FirstIRArg] = Dst; | ||
| break; | ||
| } | ||
| // No addressable source for this Indirect arg (rare; e.g. a scalar | ||
| // RValue the ABI classifies as Indirect). The fall-through below | ||
| // would create a current-frame byval-temp that dangles past the | ||
| // tail-call teardown. Refuse cleanly; codegen continues producing | ||
| // IR but the error prevents it from reaching the backend. | ||
|
xroche marked this conversation as resolved.
|
||
| CGM.getDiags().Report(MustTailCall->getBeginLoc(), | ||
| diag::err_musttail_unsupported_indirect_arg); | ||
| } | ||
|
|
||
| if (I->isAggregate()) { | ||
| // We want to avoid creating an unnecessary temporary+copy here; | ||
| // however, we need one in three cases: | ||
|
|
@@ -6083,6 +6186,13 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, | |
| } | ||
| } | ||
|
|
||
| // Phase 2 of the musttail Indirect-arg copy: flush each captured scratch | ||
| // into its incoming-param destination. Phase 1 has read every source, so | ||
| // permutations like C(b, a) land the right value in each slot. | ||
| for (const auto &Copy : MustTailIndirectCopies) | ||
| EmitAggregateCopy(Copy.Dst, Copy.Scratch, Copy.Ty, | ||
| AggValueSlot::DoesNotOverlap); | ||
|
|
||
| const CGCallee &ConcreteCallee = Callee.prepareConcreteCallee(*this); | ||
| llvm::Value *CalleePtr = ConcreteCallee.getFunctionPointer(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // RUN: %clang_cc1 -triple=x86_64-linux-gnu -verify -emit-llvm-only %s | ||
| // RUN: %clang_cc1 -triple=riscv64-linux-gnu -verify -emit-llvm-only %s | ||
|
|
||
| // A musttail Indirect argument is forwarded through the matching incoming | ||
| // parameter, which requires an in-memory source. A wide _BitInt has scalar | ||
| // evaluation kind, so the argument is a scalar value with no source storage | ||
| // to forward, regardless of value category. Such a call is rejected rather | ||
| // than routed through a caller-frame temp that dangles past the tail call. | ||
|
xroche marked this conversation as resolved.
|
||
|
|
||
| typedef _BitInt(256) BI; | ||
| BI cee(BI x); | ||
| BI pee(BI a) { | ||
| // expected-error@+1 {{'musttail' call requires passing an argument by reference, but the source does not have an addressable storage and would alias the caller's frame}} | ||
| __attribute__((musttail)) return cee(a); | ||
| } | ||
|
|
||
| // An aggregate lvalue has addressable storage to forward, so it is accepted. | ||
| // Confirms the diagnostic is specific to the no-source case. | ||
| struct Big { | ||
| unsigned long long a, b, c, d; | ||
| }; | ||
| struct Big cee_ok(struct Big x); | ||
| struct Big pee_ok(struct Big a) { | ||
| __attribute__((musttail)) return cee_ok(a); | ||
| } | ||
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.
Maybe we should split this off into a followup patch, so it's easier to bisect if there are any issues.
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.
Humm yes. I could keep the musttail-gated forwarding here (the musttail fix needs it for C++ arguments) and move the "forward for all calls" generalization, with its test churn and the exclusion-list question, to a follow-up PR. WHat do you think ?