Page MenuHomePhabricator

Implement X86 code generation for musttail
ClosedPublic

Authored by rnk on Apr 24 2014, 3:29 PM.

Details

Summary

Currently, musttail codegen is relying on sibcall optimization, and
reporting a fatal error if fails. Sibcall optimization fails when stack
arguments need to be modified, which is insufficient for musttail.

The logic for moving arguments in memory safely is already implemented
for GuaranteedTailCallOpt. This change merely arranges for musttail
calls to use it.

No functional change for GuaranteedTailCallOpt.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 8819.Apr 24 2014, 3:29 PM
rnk retitled this revision from to Implement X86 code generation for musttail.
rnk updated this object.
rnk added a subscriber: Unknown Object (MLST).

Would it be possible to instead change IsEligibleForTailCallOptimization to allow sib calls in cases where arguments have to be updated? For example, we should be able to consider t4 in musttail.ll to have a sibcall, no?

lib/Target/X86/X86ISelLowering.cpp
2538 ↗(On Diff #8819)

This is redundant with the following if. Maybe just leaving this if as is and moving the IsMustTail definition down would be easier to read?

2546 ↗(On Diff #8819)

IsSibcall is already false in here.

rnk added a comment.Apr 29 2014, 11:19 AM

Would it be possible to instead change IsEligibleForTailCallOptimization to allow sib calls in cases where arguments have to be updated?

Maybe. For musttail, we always have to be able to eliminate the tail call, regardless of what IsEligibleForTailCallOptimization says.

I started this change by making IsEligibleForTailCallOptimization return true in more cases, but ultimately I gave when I encountered MatchingStackOffset(). Evan's vision for sibcall optimization seems to be that it won't form any load and store DAG nodes. If you go back to the old llvmdev discussion about it, he felt that -tailcallopt, or at least the DAG that it created, wasn't profitable.

IMO we could revise that to: don't do sibcall optimization if we have to move the return address, and don't generate loads and stores to move parameters if we can safely detect that we don't have to (i.e. what MatchingStackOffset does).

For example, we should be able to consider t4 in musttail.ll to have a sibcall, no?

That would be a large, separate change for sibcall optimization, so I skipped it.

Do you think it's important to finish off this sibcall improvement before continuing with musttail, or can it wait?

lib/Target/X86/X86ISelLowering.cpp
2538 ↗(On Diff #8819)

I agree

For example, we should be able to consider t4 in musttail.ll to have a sibcall, no?

That would be a large, separate change for sibcall optimization, so I skipped it.

Do you think it's important to finish off this sibcall improvement before continuing with musttail, or can it wait?

No, you have a good point. Changing sibcall is an optimization, so
there are treadoffs to be discussed. With musttail that is a
correctness problem and we can get that first.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2538
@@ -2536,3 +2537,3 @@

  • if (MF.getTarget().Options.DisableTailCalls) + if (!IsMustTail && MF.getTarget().Options.DisableTailCalls) isTailCall = false; ---------------- Rafael Ávila de Espíndola wrote:

This is redundant with the following if. Maybe just leaving this if as is and moving the IsMustTail definition down would be easier to read?

I agree

LGTM with the nits fixed.

rnk closed this revision.May 19 2014, 7:23 AM
rnk updated this revision to Diff 9546.

Closed by commit rL207598 (authored by @rnk).