Skip to content

SILInliner: Initial support for begin_apply #17026

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 7, 2018
Merged

SILInliner: Initial support for begin_apply #17026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcd9ada
Cleanup unused variable
aschwaighofer Jun 1, 2018
dfec52b
SILInliner: Initial support for begin_apply
aschwaighofer Jun 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions lib/SILOptimizer/Utils/PerformanceInlinerUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,12 +656,7 @@ static bool isCallerAndCalleeLayoutConstraintsCompatible(FullApplySite AI) {
// Returns the callee of an apply_inst if it is basically inlinable.
SILFunction *swift::getEligibleFunction(FullApplySite AI,
InlineSelection WhatToInline) {
// For now, we cannot inline begin_apply at all.
if (isa<BeginApplyInst>(AI))
return nullptr;

SILFunction *Callee = AI.getReferencedFunction();
SILFunction *EligibleCallee = nullptr;

if (!Callee) {
return nullptr;
Expand Down Expand Up @@ -776,8 +771,7 @@ SILFunction *swift::getEligibleFunction(FullApplySite AI,
return nullptr;
}

EligibleCallee = Callee;
return EligibleCallee;
return Callee;
}

/// Returns true if the instruction \I has any interesting side effects which
Expand Down
215 changes: 206 additions & 9 deletions lib/SILOptimizer/Utils/SILInliner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,181 @@
using namespace swift;

bool SILInliner::canInlineFunction(FullApplySite AI) {
// For now, we cannot inline begin_apply at all.
if (isa<BeginApplyInst>(AI))
return false;

return AI.getFunction() != &Original;
}

/// Utility class for rewiring control-flow of inlined begin_apply functions.
class BeginApplySite {
SmallVector<SILBasicBlock *, 4> ExitingBlocks;
SmallVector<AllocStackInst*, 8> YieldedIndirectValues;
SILLocation Loc;
SILBuilder &Builder;
BeginApplyInst *BeginApply;
SILFunction *F;
EndApplyInst *EndApply = nullptr;
SILBasicBlock *EndApplyBB = nullptr;
SILBasicBlock *EndApplyBBMerge = nullptr;
AbortApplyInst *AbortApply = nullptr;
SILBasicBlock *AbortApplyBB = nullptr;
SILBasicBlock *AbortApplyBBMerge = nullptr;
SILArgument *IntToken = nullptr;

unsigned YieldNum = 0;
SmallVector<SILBasicBlock*, 8> YieldResumes;
SmallVector<SILBasicBlock*, 8> YieldUnwinds;

void
getYieldCaseBBs(SmallVectorImpl<std::pair<SILValue, SILBasicBlock *>> &Result,
SmallVectorImpl<SILBasicBlock *> &Dests) {
unsigned Token = 0;
for (auto *Blk : Dests) {
Result.push_back(std::make_pair(
SILValue(Builder.createIntegerLiteral(
Loc,
SILType::getBuiltinIntegerType(
32, Builder.getFunction().getModule().getASTContext()),
Token++)),
Blk));
}
}

public:
BeginApplySite(BeginApplyInst *BeginApply, SILLocation Loc,
SILBuilder &Builder)
: Loc(Loc), Builder(Builder), BeginApply(BeginApply),
F(BeginApply->getFunction()) {}

static Optional<BeginApplySite> isa(FullApplySite AI, SILLocation Loc,
SILBuilder &Builder) {
auto *BeginApply = dyn_cast<BeginApplyInst>(AI);
if (!BeginApply)
return None;
return BeginApplySite(BeginApply, Loc, Builder);
}

void collectCallerExitingBlocks() {
F->findExitingBlocks(ExitingBlocks);
}

void processApply(SILBasicBlock *ReturnToBB) {
// Handle direct and indirect results.
for (auto YieldedValue : BeginApply->getYieldedValues()) {
// Insert an alloc_stack for indirect results.
if (YieldedValue->getType().isAddress()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a placeholder? Because this shouldn't be necessary — the indirect yield comes from a value that should be live after inlining — and it's not semantically valid to introduce a copy if e.g. the yielded value is inout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this under the assumption we yield indirect values only @in.

I agree. I could have just used the inlined alloc_stack instead.

I am not sure how inout yields work. What is being yielded on the callee side? I would assume the following: We create a temporary location in the callee that is then replaced by the caller's location if we inline. Is the following a correct use of an inout yield?

Caller:

%caller_loc = alloc_stack
copy_addr %something to [initialization] %caller_loc
(%caller_loc, %token)= begin_apply %coroutine()

Callee:

sil [transparent] @coroutine : $@yield_once() -> (@yields @inout Something) {
 entry:
   %temp = alloc_stack $Something
   copy_addr %something to %temp
   yield %temp : $*Something, resume resume, unwind unwind

resume:
   apply %some_use(%temp)

Couldn't this just as well be expressed as a parameter that we pass inout to the begin_apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right the interesting case is this:

Caller:

(%caller_loc, %token)= begin_apply %coroutine()
copy_addr %something to %caller_loc // This is now available on the resume path of the callee.

Copy link
Contributor

@rjmccall rjmccall Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should think of the yield like a function call. The coroutine can produce any address it wants as the yield value, and the caller just receives an opaque pointer — just like a function can pass any address it wants as an inout argument, and the callee just receives an opaque pointer.

The convention on the yielded address is what the caller is expected to have done before it resumes the coroutine; it becomes invalid after the coroutine resumes. Among other things, this means that if the caller wants to save a value that was yielded @in to it, it's required to move the value into caller-owned memory. In the inliner, you aren't reordering code in the caller, so you can just assume that kind of lifetime issue is handled correctly; it's the code-motion and copy-propagation optimizations that will have to worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks.

Fixed both issues in #18477

Builder.setInsertionPoint(F->getEntryBlock()->begin());
auto Addr = Builder.createAllocStack(
Loc, YieldedValue->getType().getObjectType());
YieldedValue->replaceAllUsesWith(Addr);
YieldedIndirectValues.push_back(Addr);
for (auto *Exit : ExitingBlocks) {
Builder.setInsertionPoint(Exit->getTerminator());
Builder.createDeallocStack(Loc, Addr);
}
continue;
}
// Insert a phi for direct results.
auto *RetArg = ReturnToBB->createPHIArgument(YieldedValue->getType(),
ValueOwnershipKind::Owned);
// Replace all uses of the ApplyInst with the new argument.
YieldedValue->replaceAllUsesWith(RetArg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking that you'll phi together all the yield sites? Because that's going to break a lot of structural properties in the inlined coroutine if there are values live across the yield. If there are multiple yield sites, you just have to clone code in the caller in general. It might be okay to just not inline these cases in the short term, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overlooked that. You are correct that I have to disallow inlining if the values live across the yield are not dominating all yields with the current approach (or I think I could rewrite the code such that's they to dominate for address values and add phis and undefs for direct values). I tried not to replicate caller code.

}

// Add a trailing phi argument for the token integer (tells us which yield
// we came from).
IntToken = ReturnToBB->createPHIArgument(
SILType::getBuiltinIntegerType(32, F->getModule().getASTContext()),
ValueOwnershipKind::Owned);

// Get the end_apply, abort_apply instructions.
auto Token = BeginApply->getTokenResult();
for (auto *TokenUse : Token->getUses()) {
EndApply = dyn_cast<EndApplyInst>(TokenUse->getUser());
if (EndApply)
continue;
AbortApply = cast<AbortApplyInst>(TokenUse->getUser());
}

// Split the basic block before the end/abort_apply. We will insert code
// to jump to the resume/unwind blocks depending on the integer token
// later. And the inlined resume/unwind return blocks will jump back to
// the merge blocks.
EndApplyBB = EndApply->getParent();
EndApplyBBMerge = EndApplyBB->split(SILBasicBlock::iterator(EndApply));
if (AbortApply) {
AbortApplyBB = AbortApply->getParent();
AbortApplyBBMerge =
AbortApplyBB->split(SILBasicBlock::iterator(AbortApply));
}
}

void processTerminator(
TermInst *Terminator, SILBasicBlock *ReturnToBB,
llvm::function_ref<SILBasicBlock *(SILBasicBlock *)> remapBlock,
llvm::function_ref<SILValue(SILValue)> remapValue,
llvm::function_ref<void(TermInst *)> mapTerminator) {
// A yield branches to the begin_apply return block passing the yielded
// results as branch arguments. Collect the yields target block for
// resuming later. Pass an integer token to the begin_apply return block
// to mark the yield we came from.
if (auto *Yield = dyn_cast<YieldInst>(Terminator)) {
YieldResumes.push_back(remapBlock(Yield->getResumeBB()));
YieldUnwinds.push_back(remapBlock(Yield->getUnwindBB()));
auto ContextToken = Builder.createIntegerLiteral(
Loc,
SILType::getBuiltinIntegerType(32, F->getModule().getASTContext()),
YieldNum++);

SmallVector<SILValue, 8> BrResults;
unsigned IndirectIdx = 0;
for (auto CalleeYieldedVal : Yield->getYieldedValues()) {
auto YieldedVal = remapValue(CalleeYieldedVal);
if (YieldedVal->getType().isAddress()) {
auto YieldedDestAddr = YieldedIndirectValues[IndirectIdx++];
Builder.createCopyAddr(Loc, YieldedVal, YieldedDestAddr, IsTake,
IsInitialization);
} else
BrResults.push_back(YieldedVal);
}
BrResults.push_back(SILValue(ContextToken));
Builder.createBranch(Loc, ReturnToBB, BrResults);
return;
}

// Return and unwind terminators branch to the end_apply/abort_apply merge
// block respectively.
if (auto *RI = dyn_cast<ReturnInst>(Terminator)) {
Builder.createBranch(Loc, EndApplyBBMerge);
return;
}
if (auto *Unwind = dyn_cast<UnwindInst>(Terminator)) {
Builder.createBranch(Loc, AbortApplyBBMerge);
return;
}

// Otherwise, we just map the branch instruction.
assert(!::isa<ThrowInst>(Terminator) &&
"Unexpected throw instruction in yield_once function");
mapTerminator(Terminator);
}

void dispatchToResumeUnwindBlocks() {
// Resume edge.
Builder.setInsertionPoint(EndApplyBB);
SmallVector<std::pair<SILValue, SILBasicBlock *>, 8> CaseBBs;
getYieldCaseBBs(CaseBBs, YieldResumes);
Builder.createSwitchValue(Loc, IntToken, nullptr, CaseBBs);
EndApply->eraseFromParent();
// Unwind edge.
if (AbortApplyBB) {
Builder.setInsertionPoint(AbortApplyBB);
SmallVector<std::pair<SILValue, SILBasicBlock *>, 8> CaseBBs;
getYieldCaseBBs(CaseBBs, YieldUnwinds);
Builder.createSwitchValue(Loc, IntToken, nullptr, CaseBBs);
AbortApply->eraseFromParent();
}
}
};

/// \brief Inlines the callee of a given ApplyInst (which must be the value of a
/// FunctionRefInst referencing a function with a known body), into the caller
/// containing the ApplyInst, which must be the same function as provided to the
Expand Down Expand Up @@ -114,6 +282,12 @@ void SILInliner::inlineFunction(FullApplySite AI, ArrayRef<SILValue> Args) {
ValueMap.insert(std::make_pair(calleeArg, callArg));
}

// Find the existing blocks. We will need them for inlining the co-routine
// call.
auto BeginApply = BeginApplySite::isa(AI, Loc.getValue(), getBuilder());
if (BeginApply)
BeginApply->collectCallerExitingBlocks();

// Recursively visit callee's BB in depth-first preorder, starting with the
// entry block, cloning all instructions other than terminators.
visitSILBasicBlock(CalleeEntryBB);
Expand All @@ -131,6 +305,7 @@ void SILInliner::inlineFunction(FullApplySite AI, ArrayRef<SILValue> Args) {

// If we're inlining into a try_apply, we already have a return-to BB.
SILBasicBlock *ReturnToBB;

if (auto tryAI = dyn_cast<TryApplyInst>(AI)) {
ReturnToBB = tryAI->getNormalBB();

Expand All @@ -149,17 +324,35 @@ void SILInliner::inlineFunction(FullApplySite AI, ArrayRef<SILValue> Args) {
SILFunction::iterator(ReturnToBB));

// Create an argument on the return-to BB representing the returned value.
auto apply = cast<ApplyInst>(AI.getInstruction());
auto *RetArg = ReturnToBB->createPHIArgument(apply->getType(),
ValueOwnershipKind::Owned);
// Replace all uses of the ApplyInst with the new argument.
apply->replaceAllUsesWith(RetArg);
if (auto apply = dyn_cast<ApplyInst>(AI.getInstruction())) {
auto *RetArg = ReturnToBB->createPHIArgument(apply->getType(),
ValueOwnershipKind::Owned);
// Replace all uses of the ApplyInst with the new argument.
apply->replaceAllUsesWith(RetArg);
} else {
// Handle begin_apply.
BeginApply->processApply(ReturnToBB);
}
}

// Now iterate over the callee BBs and fix up the terminators.
for (auto BI = BBMap.begin(), BE = BBMap.end(); BI != BE; ++BI) {
getBuilder().setInsertionPoint(BI->second);

// Coroutine terminators need special handling.
if (BeginApply) {
BeginApply->processTerminator(
BI->first->getTerminator(), ReturnToBB,
[=](SILBasicBlock *Block) -> SILBasicBlock * {
return this->remapBasicBlock(Block);
},
[=](SILValue Val) -> SILValue {
return this->remapValue(Val);
},
[=](TermInst *Term) { this->visit(Term); });
continue;
}

// Modify return terminators to branch to the return-to BB, rather than
// trying to clone the ReturnInst.
if (auto *RI = dyn_cast<ReturnInst>(BI->first->getTerminator())) {
Expand Down Expand Up @@ -190,6 +383,10 @@ void SILInliner::inlineFunction(FullApplySite AI, ArrayRef<SILValue> Args) {
// but remaps basic blocks and values.
visit(BI->first->getTerminator());
}

// Insert dispatch code at end/abort_apply to the resume/unwind target blocks.
if (BeginApply)
BeginApply->dispatchToResumeUnwindBlocks();
}

SILValue SILInliner::borrowFunctionArgument(SILValue callArg,
Expand Down
Loading