Skip to content

Commit

Permalink
[coroutine] Fixes "cannot move instruction since its users are not do…
Browse files Browse the repository at this point in the history 
…minated by CoroBegin" problem.

Summary:
Fixes https://bugs.llvm.org/show_bug.cgi?id=36578 and https://bugs.llvm.org/show_bug.cgi?id=36296.
Supersedes: https://reviews.llvm.org/D55966

One of the fundamental transformation that CoroSplit pass performs before splitting the coroutine is to find which values need to survive between suspend and resume and provide a slot for them in the coroutine frame to spill and restore the value as needed.

Coroutine frame becomes available once the storage for it was allocated and that point is marked in the pre-split coroutine with a llvm.coro.begin intrinsic.

FE normally puts all of the user-authored code that would be accessing those values after llvm.coro.begin, however, sometimes instructions accessing those values would end up prior to coro.begin. For example, writing out a value of the parameter into the alloca done by the FE or instructions that are added by the optimization passes such as SROA when it rewrites allocas.

Prior to this change, CoroSplit pass would try to move instructions that may end up accessing the values in the coroutine frame after CoroBegin. However it would run into problems (report_fatal_error) if some of the values would be used both in the allocation function (for example allocator is passed as a parameter to a coroutine) and in the use-authored body of the coroutine.

To handle this case and to simplify the instruction moving logic, this change removes all of the instruction moving. Instead, we only change the uses of the spilled values that are dominated by coro.begin and leave other instructions intact.

Before:

```
%var = alloca i32
%1 = getelementptr .. %var; ; will move this one after coro.begin
%f = call i8* @llvm.coro.begin(
```

After:

```
%var = alloca i32
%1 = getelementptr .. %var; stays put
%f = call i8* @llvm.coro.begin(
```
If we discover that there is a potential write into an alloca, prior to coro.begin we would copy its value from the alloca into the spill slot in the coroutine frame.

Before:

```
%var = alloca i32
store .. %var ; will move this one after coro.begin
%f = call i8* @llvm.coro.begin(
```

After:

```
%var = alloca i32
store .. %var ;stays put
%f = call i8* @llvm.coro.begin(
%tmp = load %var
store %tmp, %spill.slot.for.var
```

Note: This change does not handle array allocas as that is something that C++ FE does not produce, but, it can be added in the future if need arises

Reviewers: llvm-commits, modocache, ben-clayton, tks2103, rjmccall

Reviewed By: modocache

Subscribers: bartdesmet

Differential Revision: https://reviews.llvm.org/D66230

llvm-svn: 368949
  • Loading branch information
@GorNishanov
GorNishanov committed Aug 15, 2019
1 parent 399408a commit efe0093
Show file tree
Hide file tree
Showing 4 changed files with 270 additions and 148 deletions.
163 changes: 105 additions & 58 deletions llvm/lib/Transforms/Coroutines/CoroFrame.cpp
View file
Expand Up @@ -18,6 +18,7 @@

#include "CoroInternal.h"
#include "llvm/ADT/BitVector.h"
#include "llvm/Analysis/PtrUseVisitor.h"
#include "llvm/Transforms/Utils/Local.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/IR/CFG.h"
Expand Down Expand Up @@ -484,6 +485,61 @@ static StructType *buildFrameType(Function &F, coro::Shape &Shape,
return FrameTy;
}

// We use a pointer use visitor to discover if there are any writes into an
// alloca that dominates CoroBegin. If that is the case, insertSpills will copy
// the value from the alloca into the coroutine frame spill slot corresponding
// to that alloca.
namespace {
struct AllocaUseVisitor : PtrUseVisitor<AllocaUseVisitor> {
using Base = PtrUseVisitor<AllocaUseVisitor>;
AllocaUseVisitor(const DataLayout &DL, const DominatorTree &DT,
const CoroBeginInst &CB)
: PtrUseVisitor(DL), DT(DT), CoroBegin(CB) {}

// We are only interested in uses that dominate coro.begin.
void visit(Instruction &I) {
if (DT.dominates(&I, &CoroBegin))
Base::visit(I);
}
// We need to provide this overload as PtrUseVisitor uses a pointer based
// visiting function.
void visit(Instruction *I) { return visit(*I); }

void visitLoadInst(LoadInst &) {} // Good. Nothing to do.

// If the use is an operand, the pointer escaped and anything can write into
// that memory. If the use is the pointer, we are definitely writing into the
// alloca and therefore we need to copy.
void visitStoreInst(StoreInst &SI) { PI.setAborted(&SI); }

// Any other instruction that is not filtered out by PtrUseVisitor, will
// result in the copy.
void visitInstruction(Instruction &I) { PI.setAborted(&I); }

private:
const DominatorTree &DT;
const CoroBeginInst &CoroBegin;
};
} // namespace
static bool mightWriteIntoAllocaPtr(AllocaInst &A, const DominatorTree &DT,
const CoroBeginInst &CB) {
const DataLayout &DL = A.getModule()->getDataLayout();
AllocaUseVisitor Visitor(DL, DT, CB);
auto PtrI = Visitor.visitPtr(A);
if (PtrI.isEscaped() || PtrI.isAborted()) {
auto *PointerEscapingInstr = PtrI.getEscapingInst()
? PtrI.getEscapingInst()
: PtrI.getAbortingInst();
if (PointerEscapingInstr) {
LLVM_DEBUG(
dbgs() << "AllocaInst copy was triggered by instruction: "
<< *PointerEscapingInstr << "\n");
}
return true;
}
return false;
}

// We need to make room to insert a spill after initial PHIs, but before
// catchswitch instruction. Placing it before violates the requirement that
// catchswitch, like all other EHPads must be the first nonPHI in a block.
Expand Down Expand Up @@ -535,6 +591,7 @@ static Instruction *insertSpills(const SpillInfo &Spills, coro::Shape &Shape) {
PointerType *FramePtrTy = FrameTy->getPointerTo();
auto *FramePtr =
cast<Instruction>(Builder.CreateBitCast(CB, FramePtrTy, "FramePtr"));
DominatorTree DT(*CB->getFunction());

Value *CurrentValue = nullptr;
BasicBlock *CurrentBlock = nullptr;
Expand Down Expand Up @@ -641,11 +698,17 @@ static Instruction *insertSpills(const SpillInfo &Spills, coro::Shape &Shape) {
// that the suspend will be followed by a branch.
InsertPt = CSI->getParent()->getSingleSuccessor()->getFirstNonPHI();
} else {
auto *I = cast<Instruction>(E.def());
assert(!I->isTerminator() && "unexpected terminator");
// For all other values, the spill is placed immediately after
// the definition.
assert(!cast<Instruction>(E.def())->isTerminator() &&
"unexpected terminator");
InsertPt = cast<Instruction>(E.def())->getNextNode();
if (DT.dominates(CB, I)) {
InsertPt = I->getNextNode();
} else {
// Unless, it is not dominated by CoroBegin, then it will be
// inserted immediately after CoroFrame is computed.
InsertPt = FramePtr->getNextNode();
}
}

Builder.SetInsertPoint(InsertPt);
Expand Down Expand Up @@ -683,17 +746,48 @@ static Instruction *insertSpills(const SpillInfo &Spills, coro::Shape &Shape) {
FramePtrBB->splitBasicBlock(FramePtr->getNextNode(), "AllocaSpillBB");
SpillBlock->splitBasicBlock(&SpillBlock->front(), "PostSpill");
Shape.AllocaSpillBlock = SpillBlock;

// If we found any allocas, replace all of their remaining uses with Geps.
Builder.SetInsertPoint(&SpillBlock->front());
// Note: we cannot do it indiscriminately as some of the uses may not be
// dominated by CoroBegin.
bool MightNeedToCopy = false;
Builder.SetInsertPoint(&Shape.AllocaSpillBlock->front());
SmallVector<Instruction *, 4> UsersToUpdate;
for (auto &P : Allocas) {
auto *G = GetFramePointer(P.second, P.first);
AllocaInst *const A = P.first;
UsersToUpdate.clear();
for (User *U : A->users()) {
auto *I = cast<Instruction>(U);
if (DT.dominates(CB, I))
UsersToUpdate.push_back(I);
else
MightNeedToCopy = true;
}
if (!UsersToUpdate.empty()) {
auto *G = GetFramePointer(P.second, A);
G->takeName(A);
for (Instruction *I : UsersToUpdate)
I->replaceUsesOfWith(A, G);
}
}
// If we discovered such uses not dominated by CoroBegin, see if any of them
// preceed coro begin and have instructions that can modify the
// value of the alloca and therefore would require a copying the value into
// the spill slot in the coroutine frame.
if (MightNeedToCopy) {
Builder.SetInsertPoint(FramePtr->getNextNode());

for (auto &P : Allocas) {
AllocaInst *const A = P.first;
if (mightWriteIntoAllocaPtr(*A, DT, *CB)) {
if (A->isArrayAllocation())
report_fatal_error(
"Coroutines cannot handle copying of array allocas yet");

// We are not using ReplaceInstWithInst(P.first, cast<Instruction>(G)) here,
// as we are changing location of the instruction.
G->takeName(P.first);
P.first->replaceAllUsesWith(G);
P.first->eraseFromParent();
auto *G = GetFramePointer(P.second, A);
auto *Value = Builder.CreateLoad(A);
Builder.CreateStore(Value, G);
}
}
}
return FramePtr;
}
Expand Down Expand Up @@ -895,52 +989,6 @@ static void rewriteMaterializableInstructions(IRBuilder<> &IRB,
}
}

// Move early uses of spilled variable after CoroBegin.
// For example, if a parameter had address taken, we may end up with the code
// like:
// define @f(i32 %n) {
// %n.addr = alloca i32
// store %n, %n.addr
// ...
// call @coro.begin
// we need to move the store after coro.begin
static void moveSpillUsesAfterCoroBegin(Function &F, SpillInfo const &Spills,
CoroBeginInst *CoroBegin) {
DominatorTree DT(F);
SmallVector<Instruction *, 8> NeedsMoving;

Value *CurrentValue = nullptr;

for (auto const &E : Spills) {
if (CurrentValue == E.def())
continue;

CurrentValue = E.def();

for (User *U : CurrentValue->users()) {
Instruction *I = cast<Instruction>(U);
if (!DT.dominates(CoroBegin, I)) {
LLVM_DEBUG(dbgs() << "will move: " << *I << "\n");

// TODO: Make this more robust. Currently if we run into a situation
// where simple instruction move won't work we panic and
// report_fatal_error.
for (User *UI : I->users()) {
if (!DT.dominates(CoroBegin, cast<Instruction>(UI)))
report_fatal_error("cannot move instruction since its users are not"
" dominated by CoroBegin");
}

NeedsMoving.push_back(I);
}
}
}

Instruction *InsertPt = CoroBegin->getNextNode();
for (Instruction *I : NeedsMoving)
I->moveBefore(InsertPt);
}

// Splits the block at a particular instruction unless it is the first
// instruction in the block with a single predecessor.
static BasicBlock *splitBlockIfNotFirst(Instruction *I, const Twine &Name) {
Expand Down Expand Up @@ -1383,7 +1431,6 @@ void coro::buildCoroutineFrame(Function &F, Shape &Shape) {
}
}
LLVM_DEBUG(dump("Spills", Spills));
moveSpillUsesAfterCoroBegin(F, Spills, Shape.CoroBegin);
Shape.FrameTy = buildFrameType(F, Shape, Spills);
Shape.FramePtr = insertSpills(Spills, Shape);
lowerLocalAllocas(LocalAllocas, DeadInstructions);
Expand Down
90 changes: 0 additions & 90 deletions llvm/lib/Transforms/Coroutines/CoroSplit.cpp
View file
Expand Up @@ -1153,95 +1153,6 @@ static void simplifySuspendPoints(coro::Shape &Shape) {
S.resize(N);
}

static SmallPtrSet<BasicBlock *, 4> getCoroBeginPredBlocks(CoroBeginInst *CB) {
// Collect all blocks that we need to look for instructions to relocate.
SmallPtrSet<BasicBlock *, 4> RelocBlocks;
SmallVector<BasicBlock *, 4> Work;
Work.push_back(CB->getParent());

do {
BasicBlock *Current = Work.pop_back_val();
for (BasicBlock *BB : predecessors(Current))
if (RelocBlocks.count(BB) == 0) {
RelocBlocks.insert(BB);
Work.push_back(BB);
}
} while (!Work.empty());
return RelocBlocks;
}

static SmallPtrSet<Instruction *, 8>
getNotRelocatableInstructions(CoroBeginInst *CoroBegin,
SmallPtrSetImpl<BasicBlock *> &RelocBlocks) {
SmallPtrSet<Instruction *, 8> DoNotRelocate;
// Collect all instructions that we should not relocate
SmallVector<Instruction *, 8> Work;

// Start with CoroBegin and terminators of all preceding blocks.
Work.push_back(CoroBegin);
BasicBlock *CoroBeginBB = CoroBegin->getParent();
for (BasicBlock *BB : RelocBlocks)
if (BB != CoroBeginBB)
Work.push_back(BB->getTerminator());

// For every instruction in the Work list, place its operands in DoNotRelocate
// set.
do {
Instruction *Current = Work.pop_back_val();
LLVM_DEBUG(dbgs() << "CoroSplit: Will not relocate: " << *Current << "\n");
DoNotRelocate.insert(Current);
for (Value *U : Current->operands()) {
auto *I = dyn_cast<Instruction>(U);
if (!I)
continue;

if (auto *A = dyn_cast<AllocaInst>(I)) {
// Stores to alloca instructions that occur before the coroutine frame
// is allocated should not be moved; the stored values may be used by
// the coroutine frame allocator. The operands to those stores must also
// remain in place.
for (const auto &User : A->users())
if (auto *SI = dyn_cast<llvm::StoreInst>(User))
if (RelocBlocks.count(SI->getParent()) != 0 &&
DoNotRelocate.count(SI) == 0) {
Work.push_back(SI);
DoNotRelocate.insert(SI);
}
continue;
}

if (DoNotRelocate.count(I) == 0) {
Work.push_back(I);
DoNotRelocate.insert(I);
}
}
} while (!Work.empty());
return DoNotRelocate;
}

static void relocateInstructionBefore(CoroBeginInst *CoroBegin, Function &F) {
// Analyze which non-alloca instructions are needed for allocation and
// relocate the rest to after coro.begin. We need to do it, since some of the
// targets of those instructions may be placed into coroutine frame memory
// for which becomes available after coro.begin intrinsic.

auto BlockSet = getCoroBeginPredBlocks(CoroBegin);
auto DoNotRelocateSet = getNotRelocatableInstructions(CoroBegin, BlockSet);

Instruction *InsertPt = CoroBegin->getNextNode();
BasicBlock &BB = F.getEntryBlock(); // TODO: Look at other blocks as well.
for (auto B = BB.begin(), E = BB.end(); B != E;) {
Instruction &I = *B++;
if (isa<AllocaInst>(&I))
continue;
if (&I == CoroBegin)
break;
if (DoNotRelocateSet.count(&I))
continue;
I.moveBefore(InsertPt);
}
}

static void splitSwitchCoroutine(Function &F, coro::Shape &Shape,
SmallVectorImpl<Function *> &Clones) {
assert(Shape.ABI == coro::ABI::Switch);
Expand Down Expand Up @@ -1441,7 +1352,6 @@ static void splitCoroutine(Function &F, CallGraph &CG, CallGraphSCC &SCC) {
return;

simplifySuspendPoints(Shape);
relocateInstructionBefore(Shape.CoroBegin, F);
buildCoroutineFrame(F, Shape);
replaceFrameSize(Shape);

Expand Down

0 comments on commit efe0093

Please sign in to comment.