This is an archive of the discontinued LLVM Phabricator instance.

Differential D11476

Marked call instruction in thunk function with tail attribute when applicable
ClosedPublic

Authored by aaboud on Jul 23 2015, 2:15 PM.

Details

Reviewers
hans
rnk
Commits
rG819ad33f636e: Mark calls in thunk functions as tail-call optimization candidates
rC244207: Mark calls in thunk functions as tail-call optimization candidates
rL244207: Mark calls in thunk functions as tail-call optimization candidates
Summary

This patch fix PR24235.
When thunk function is being generated with a call to the original adjusted function, the thunk function will appear in the call stack of the debugger.
To solve this issue, the thunk function should try replace the call instruction with a jump instruction.

This patch mark the call function with tail attribute, when it is applicable, to allow the backend to replace the call with a jump.

Notice that marking the call with a tail might be safer, however, it is not good enough to remove the call in all cases, e.g. when the calling convention is not fastcc.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 30524.Jul 23 2015, 2:15 PM
aaboud retitled this revision from to Marked call instruction in thunk function with musttail attribute when applicable.
aaboud updated this object.
aaboud added reviewers: rnk, hans.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: cfe-commits.
aaboud added a comment.Aug 2 2015, 2:01 AM

I did not get any comment on this patch yet.

ABataev added a subscriber: ABataev.Aug 3 2015, 6:34 AM
ABataev added inline comments.
lib/CodeGen/CGVTables.cpp
315–316

I think it would better to improve code in lines 252-262 rather than setting musttail attribute manually.

rnk edited edge metadata.Aug 3 2015, 9:22 AM

Musttail is currently x86 only, unfortunately. I started making it work for AArch64, but it was pretty hard and I gave up to work on other things.

That aside, musttail is also somewhat optimization hostile. It interacts with the inliner in surprising ways.

All in all, I'd rather not do this.

aaboud added a comment.Aug 3 2015, 11:46 AM

Thanks for the comment.
So, do you suggest that we use "tail" instead of "musttail"?
Would it be fine to use "tail" here?

rnk added a comment.Aug 3 2015, 5:11 PM
In D11476#217006, @aaboud wrote:

Thanks for the comment.
So, do you suggest that we use "tail" instead of "musttail"?
Would it be fine to use "tail" here?

Yeah, adding tail is fine.

aaboud updated this revision to Diff 31341.Aug 5 2015, 3:33 AM
aaboud retitled this revision from Marked call instruction in thunk function with musttail attribute when applicable to Marked call instruction in thunk function with tail attribute when applicable.
aaboud updated this object.
aaboud edited edge metadata.
rnk accepted this revision.Aug 5 2015, 8:58 AM
rnk edited edge metadata.

The patch seems right, but I'm surprised there isn't more test fallout.

This revision is now accepted and ready to land.Aug 5 2015, 8:58 AM
Closed by commit rL244207: Mark calls in thunk functions as tail-call optimization candidates (authored by mkuper). · Explain WhyAug 6 2015, 4:58 AM
This revision was automatically updated to reflect the committed changes.
aaboud mentioned this in D22900: Revert r244207 - Mark calls in thunk functions as tail-call optimization.Jul 31 2016, 12:11 AM

Revision Contents

PathSize
lib/
CodeGen/
CGVTables.cpp
CGVTables.cpp (revision 242994)
2 lines
test/
CodeGenCXX/
microsoft-abi-structors.cpp
microsoft-abi-structors.cpp (revision 242994)
2 lines

Diff 30524

lib/CodeGen/CGVTables.cpp

Show First 20 Lines • Show All 306 Lines • ▼ Show 20 Lines#endif
// Now emit our call. // Now emit our call.
llvm::Instruction *CallOrInvoke; llvm::Instruction *CallOrInvoke;
RValue RV = EmitCall(*CurFnInfo, Callee, Slot, CallArgs, MD, &CallOrInvoke); RValue RV = EmitCall(*CurFnInfo, Callee, Slot, CallArgs, MD, &CallOrInvoke);
// Consider return adjustment if we have ThunkInfo. // Consider return adjustment if we have ThunkInfo.
if (Thunk && !Thunk->Return.isEmpty()) if (Thunk && !Thunk->Return.isEmpty())
RV = PerformReturnAdjustment(*this, ResultType, RV, *Thunk); RV = PerformReturnAdjustment(*this, ResultType, RV, *Thunk);
else if (llvm::CallInst* Call = dyn_cast<llvm::CallInst>(CallOrInvoke))
Call->setTailCallKind(llvm::CallInst::TCK_MustTail);
ABataevUnsubmitted
Not Done
ReplyInline Actions

I think it would better to improve code in lines 252-262 rather than setting musttail attribute manually.

ABataev: I think it would better to improve code in lines 252-262 rather than setting musttail attribute…
// Emit return. // Emit return.
if (!ResultType->isVoidType() && Slot.isNull()) if (!ResultType->isVoidType() && Slot.isNull())
CGM.getCXXABI().EmitReturnFromThunk(*this, RV, ResultType); CGM.getCXXABI().EmitReturnFromThunk(*this, RV, ResultType);
// Disable the final ARC autorelease. // Disable the final ARC autorelease.
AutoreleaseResult = false; AutoreleaseResult = false;
▲ Show 20 Lines • Show All 583 Lines • Show Last 20 Lines

test/CodeGenCXX/microsoft-abi-structors.cpp

Show First 20 Lines • Show All 170 Lines • ▼ Show 20 Lines
void foo() { void foo() {
C c; C c;
} }
// DTORS2-LABEL: define linkonce_odr x86_thiscallcc i8* @"\01??_EC@dtor_in_second_nvbase@@W3AEPAXI@Z" // DTORS2-LABEL: define linkonce_odr x86_thiscallcc i8* @"\01??_EC@dtor_in_second_nvbase@@W3AEPAXI@Z"
// DTORS2: (%"struct.dtor_in_second_nvbase::C"* %this, i32 %should_call_delete) // DTORS2: (%"struct.dtor_in_second_nvbase::C"* %this, i32 %should_call_delete)
// Do an adjustment from B* to C*. // Do an adjustment from B* to C*.
// DTORS2: getelementptr i8, i8* %{{.*}}, i32 -4 // DTORS2: getelementptr i8, i8* %{{.*}}, i32 -4
// DTORS2: bitcast i8* %{{.*}} to %"struct.dtor_in_second_nvbase::C"* // DTORS2: bitcast i8* %{{.*}} to %"struct.dtor_in_second_nvbase::C"*
// DTORS2: %[[CALL:.*]] = call x86_thiscallcc i8* @"\01??_GC@dtor_in_second_nvbase@@UAEPAXI@Z" // DTORS2: %[[CALL:.*]] = musttail call x86_thiscallcc i8* @"\01??_GC@dtor_in_second_nvbase@@UAEPAXI@Z"
// DTORS2: ret i8* %[[CALL]] // DTORS2: ret i8* %[[CALL]]
} }
namespace test2 { namespace test2 {
// Just like dtor_in_second_nvbase, except put that in a vbase of a diamond. // Just like dtor_in_second_nvbase, except put that in a vbase of a diamond.
// C's dtor is in the non-primary base. // C's dtor is in the non-primary base.
▲ Show 20 Lines • Show All 273 Lines • Show Last 20 Lines