[llvm] r332077 - [Coroutines] PR34897: Fix incorrect elisions
Brian Gesiak via llvm-commits
llvm-commits at lists.llvm.org
Thu May 10 20:12:29 PDT 2018
Author: modocache
Date: Thu May 10 20:12:28 2018
New Revision: 332077
URL: http://llvm.org/viewvc/llvm-project?rev=332077&view=rev
Log:
[Coroutines] PR34897: Fix incorrect elisions
Summary:
https://bugs.llvm.org/show_bug.cgi?id=34897 demonstrates an incorrect
coroutine frame allocation elision in the coro-elide pass. The elision
is performed on the basis that the SSA variables from all llvm.coro.begin
are directly referenced in subsequent llvm.coro.destroy instructions.
However, this ignores the fact that the function may exit through paths
that do not run these destroy instructions. In the sample program from
PR34897, for example, the llvm.coro.destroy instruction is only
executed in exception handling code. When the coroutine function exits
normally, llvm.coro.destroy is not called. Eliding the allocation in
this case causes a subsequent reference to the coroutine handle from
outside of the function to access freed memory.
To fix the issue, when finding an llvm.coro.destroy for each llvm.coro.begin,
only consider llvm.coro.destroy that are executed along non-exceptional paths.
Test Plan:
1. Download the sample program from
https://bugs.llvm.org/show_bug.cgi?id=34897, compile it with
`clang++ -fcoroutines-ts -stdlib=libc++ -std=c++1z -O2`, and run it.
It should print `"run1\ncheck1\nrun2\ncheck2"` and then exit
successfully.
2. Compile https://godbolt.org/g/mCKfnr and confirm it is still
optimized to a single instruction, 'return 1190'.
3. `check-llvm`
Reviewers: rsmith, GorNishanov, eric_niebler
Reviewed By: GorNishanov
Subscribers: andrewrk, lewissbaker, EricWF, llvm-commits
Differential Revision: https://reviews.llvm.org/D43242
Modified:
llvm/trunk/lib/Transforms/Coroutines/CoroElide.cpp
llvm/trunk/test/Transforms/Coroutines/coro-heap-elide.ll
Modified: llvm/trunk/lib/Transforms/Coroutines/CoroElide.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Coroutines/CoroElide.cpp?rev=332077&r1=332076&r2=332077&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Coroutines/CoroElide.cpp (original)
+++ llvm/trunk/lib/Transforms/Coroutines/CoroElide.cpp Thu May 10 20:12:28 2018
@@ -14,6 +14,7 @@
#include "CoroInternal.h"
#include "llvm/Analysis/AliasAnalysis.h"
#include "llvm/Analysis/InstructionSimplify.h"
+#include "llvm/IR/Dominators.h"
#include "llvm/IR/InstIterator.h"
#include "llvm/Pass.h"
#include "llvm/Support/ErrorHandling.h"
@@ -35,8 +36,8 @@ struct Lowerer : coro::LowererBase {
Lowerer(Module &M) : LowererBase(M) {}
void elideHeapAllocations(Function *F, Type *FrameTy, AAResults &AA);
- bool shouldElide() const;
- bool processCoroId(CoroIdInst *, AAResults &AA);
+ bool shouldElide(Function *F, DominatorTree &DT) const;
+ bool processCoroId(CoroIdInst *, AAResults &AA, DominatorTree &DT);
};
} // end anonymous namespace
@@ -141,33 +142,54 @@ void Lowerer::elideHeapAllocations(Funct
removeTailCallAttribute(Frame, AA);
}
-bool Lowerer::shouldElide() const {
+bool Lowerer::shouldElide(Function *F, DominatorTree &DT) const {
// If no CoroAllocs, we cannot suppress allocation, so elision is not
// possible.
if (CoroAllocs.empty())
return false;
// Check that for every coro.begin there is a coro.destroy directly
- // referencing the SSA value of that coro.begin. If the value escaped, then
- // coro.destroy would have been referencing a memory location storing that
- // value and not the virtual register.
-
- SmallPtrSet<CoroBeginInst *, 8> ReferencedCoroBegins;
+ // referencing the SSA value of that coro.begin along a non-exceptional path.
+ // If the value escaped, then coro.destroy would have been referencing a
+ // memory location storing that value and not the virtual register.
+
+ // First gather all of the non-exceptional terminators for the function.
+ SmallPtrSet<Instruction *, 8> Terminators;
+ for (BasicBlock &B : *F) {
+ auto *TI = B.getTerminator();
+ if (TI->getNumSuccessors() == 0 && !TI->isExceptional() &&
+ !isa<UnreachableInst>(TI))
+ Terminators.insert(TI);
+ }
+ // Filter out the coro.destroy that lie along exceptional paths.
+ SmallPtrSet<CoroSubFnInst *, 4> DAs;
for (CoroSubFnInst *DA : DestroyAddr) {
+ for (Instruction *TI : Terminators) {
+ if (DT.dominates(DA, TI)) {
+ DAs.insert(DA);
+ break;
+ }
+ }
+ }
+
+ // Find all the coro.begin referenced by coro.destroy along happy paths.
+ SmallPtrSet<CoroBeginInst *, 8> ReferencedCoroBegins;
+ for (CoroSubFnInst *DA : DAs) {
if (auto *CB = dyn_cast<CoroBeginInst>(DA->getFrame()))
ReferencedCoroBegins.insert(CB);
else
return false;
}
- // If size of the set is the same as total number of CoroBegins, means we
- // found a coro.free or coro.destroy mentioning a coro.begin and we can
+ // If size of the set is the same as total number of coro.begin, that means we
+ // found a coro.free or coro.destroy referencing each coro.begin, so we can
// perform heap elision.
return ReferencedCoroBegins.size() == CoroBegins.size();
}
-bool Lowerer::processCoroId(CoroIdInst *CoroId, AAResults &AA) {
+bool Lowerer::processCoroId(CoroIdInst *CoroId, AAResults &AA,
+ DominatorTree &DT) {
CoroBegins.clear();
CoroAllocs.clear();
CoroFrees.clear();
@@ -213,7 +235,7 @@ bool Lowerer::processCoroId(CoroIdInst *
replaceWithConstant(ResumeAddrConstant, ResumeAddr);
- bool ShouldElide = shouldElide();
+ bool ShouldElide = shouldElide(CoroId->getFunction(), DT);
auto *DestroyAddrConstant = ConstantExpr::getExtractValue(
Resumers,
@@ -293,14 +315,16 @@ struct CoroElide : FunctionPass {
return Changed;
AAResults &AA = getAnalysis<AAResultsWrapperPass>().getAAResults();
+ DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
for (auto *CII : L->CoroIds)
- Changed |= L->processCoroId(CII, AA);
+ Changed |= L->processCoroId(CII, AA, DT);
return Changed;
}
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.addRequired<AAResultsWrapperPass>();
+ AU.addRequired<DominatorTreeWrapperPass>();
}
StringRef getPassName() const override { return "Coroutine Elision"; }
};
Modified: llvm/trunk/test/Transforms/Coroutines/coro-heap-elide.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Coroutines/coro-heap-elide.ll?rev=332077&r1=332076&r2=332077&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/Coroutines/coro-heap-elide.ll (original)
+++ llvm/trunk/test/Transforms/Coroutines/coro-heap-elide.ll Thu May 10 20:12:28 2018
@@ -81,6 +81,39 @@ entry:
ret void
}
+; CHECK-LABEL: @callResume_PR34897_no_elision(
+define void @callResume_PR34897_no_elision(i1 %cond) {
+; CHECK-LABEL: entry:
+entry:
+; CHECK: call i8* @CustomAlloc(
+ %hdl = call i8* @f()
+; CHECK: tail call void @bar(
+ tail call void @bar(i8* %hdl)
+; CHECK: tail call void @bar(
+ tail call void @bar(i8* null)
+ br i1 %cond, label %if.then, label %if.else
+
+; CHECK-LABEL: if.then:
+if.then:
+; CHECK: call fastcc void bitcast (void (%f.frame*)* @f.resume to void (i8*)*)(i8*
+ %0 = call i8* @llvm.coro.subfn.addr(i8* %hdl, i8 0)
+ %1 = bitcast i8* %0 to void (i8*)*
+ call fastcc void %1(i8* %hdl)
+; CHECK-NEXT: call fastcc void bitcast (void (%f.frame*)* @f.destroy to void (i8*)*)(i8*
+ %2 = call i8* @llvm.coro.subfn.addr(i8* %hdl, i8 1)
+ %3 = bitcast i8* %2 to void (i8*)*
+ call fastcc void %3(i8* %hdl)
+ br label %return
+
+if.else:
+ br label %return
+
+; CHECK-LABEL: return:
+return:
+; CHECK: ret void
+ ret void
+}
+
; a coroutine start function (cannot elide heap alloc, due to second argument to
; coro.begin not pointint to coro.alloc)
define i8* @f_no_elision() personality i8* null {
More information about the llvm-commits
mailing list