Page MenuHomePhabricator

[ICP] Handling must tail calls in indirect call promotion
ClosedPublic

Authored by hoyFB on May 1 2020, 12:45 PM.

Details

Summary

Per the IR convention, a musttail call must precede a ret with an optional bitcast. This was violated by the indirect call promotion optimization which could result an IR like:

; <label>:2192:
  br i1 %2198, label %2199, label %2201, !dbg !226012, !prof !229483

; <label>:2199:                                   ; preds = %2192
  musttail call fastcc void @foo(i8* %2195), !dbg !226012
  br label %2202, !dbg !226012

; <label>:2201:                                   ; preds = %2192
  musttail call fastcc void %2197(i8* %2195), !dbg !226012
  br label %2202, !dbg !226012

; <label>:2202:                                   ; preds = %605, %2201, %2199
  ret void, !dbg !229485

This is being fixed in this change where the return statement goes together with the promoted indirect call. The code generated is like:

; <label>:2192:
  br i1 %2198, label %2199, label %2201, !dbg !226012, !prof !229483

; <label>:2199:                                   ; preds = %2192
  musttail call fastcc void @foo(i8* %2195), !dbg !226012
  ret void, !dbg !229485

; <label>:2201:                                   ; preds = %2192
  musttail call fastcc void %2197(i8* %2195), !dbg !226012
  ret void, !dbg !229485

Diff Detail

Event Timeline

hoyFB created this revision.May 1 2020, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2020, 12:45 PM
hoyFB edited the summary of this revision. (Show Details)May 1 2020, 12:46 PM
hoyFB edited the summary of this revision. (Show Details)

Can the code be refactored such that the splitting and cloning part of the code are mostly shared?

wenlei added a comment.May 2 2020, 8:45 PM

Forgot to mention earlier - this is not unique to AutoFDO, so you may want to change the title.

hoyFB added a comment.May 2 2020, 9:27 PM

Can the code be refactored such that the splitting and cloning part of the code are mostly shared?

Yes, I tried that as my first attempt. The change looks like blow and placed closed to the end of the function. Much of the diversion is from cloning the return statement and the bitcast, and also the removal of the merge block. Either way works but I like the current version a bit more. What do you think?

if (OrigInst->isMustTailCall()) {
    auto Next = &(*MergeBlock->begin());
    Value *NewRetVal = NewInst;
    // Move the optional bitcast.
    if (auto *BitCast = dyn_cast_or_null<BitCastInst>(Next)) {
      assert(BitCast->getOperand(0) == OrigInst &&
             "bitcast following musttail call must use the call");
      Next = BitCast->getNextNode();
      auto NewBitCast = BitCast->clone();
      NewBitCast->replaceUsesOfWith(OrigInst, NewInst);
      BitCast->moveBefore(ElseTerm);
      NewBitCast->insertBefore(ThenTerm);
      NewRetVal = NewBitCast;
    }
    // Move return following a musttail call as well.
    ReturnInst *Ret = dyn_cast_or_null<ReturnInst>(Next);
    assert(Ret && "musttail call must precede a ret with an optional bitcast");
    if (!Ret->getReturnValue()) {
      NewRetVal = nullptr;
    }
    auto NewRet = Ret->clone();
    if (Ret->getReturnValue())
      NewRet->replaceUsesOfWith(Ret->getReturnValue(), NewRetVal);
    Ret->moveBefore(ElseTerm);
    NewRet->insertBefore(ThenTerm);
    // Invoke instructions are terminating, so we don't need the terminator
    // instructions that were just created.
    ThenTerm->eraseFromParent();
    ElseTerm->eraseFromParent();
    if (MergeBlock->empty())
      MergeBlock->eraseFromParent();
    return *NewInst;
  }
hoyFB added a comment.May 2 2020, 9:29 PM

Forgot to mention earlier - this is not unique to AutoFDO, so you may want to change the title.

Good point. I was thinking [Codegen] ... as the title. Maybe [ICP] ...? Not sure about the convention. @davidxl What do you think? Thanks.

davidxl accepted this revision.May 2 2020, 11:03 PM

lgtm

This revision is now accepted and ready to land.May 2 2020, 11:03 PM
wenlei accepted this revision.May 3 2020, 8:20 AM

Forgot to mention earlier - this is not unique to AutoFDO, so you may want to change the title.

Good point. I was thinking [Codegen] ... as the title. Maybe [ICP] ...? Not sure about the convention. @davidxl What do you think? Thanks.

Grepping the history, I found a few titled with [ICP].., so yeah, that's used. [TailCallElim] is also used, though I don't have preference between the two. Looks good otherwise - thanks for the fix!

hoyFB retitled this revision from [AutoFDO] Handling must tail calls in indirect call promotion to [ICP] Handling must tail calls in indirect call promotion.May 3 2020, 10:16 AM
This revision was automatically updated to reflect the committed changes.

Should we merge this to 10.0.1?