This is an archive of the discontinued LLVM Phabricator instance.

Differential D50286

avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls
ClosedPublic

Authored by rsmith on Aug 3 2018, 5:07 PM.

Details

Reviewers
rjmccall
Commits
rGaa140bf164a3: Avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end…
rL338945: Avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end…
rC338945: Avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end…
Summary

When a non-extended temporary object is created in a conditional branch, the lifetime of that temporary ends outside the conditional (at the end of the full-expression). If we're inserting lifetime markers, this means we can end up generating

if (some_cond) {
  lifetime.start(&tmp);
  Tmp::Tmp(&tmp);
}
// ...
if (some_cond) {
  lifetime.end(&tmp);
}

for a full-expression containing a subexpression of the form some_cond ? Tmp().x : 0. This patch moves the lifetime start for such a temporary out of the conditional branch so that we don't need to generate an additional basic block to hold the lifetime end marker.

This is disabled if we want precise lifetime markers (for asan's stack-use-after-scope checks) or of the temporary has a non-trivial destructor (in which case we'd generate an extra basic block anyway to hold the destructor call).

Diff Detail

Repository
rC Clang

Event Timeline

rsmith created this revision.Aug 3 2018, 5:07 PM
rsmith updated this revision to Diff 159143.Aug 3 2018, 5:23 PM

Add forgotten test file.

rjmccall added inline comments.Aug 3 2018, 6:10 PM
lib/CodeGen/CGExpr.cpp
521

Why only when optimization is enabled? This seems like a nice improvement regardless.

Also, please mention why this is disabled for destructed types in the comment, not just the commit message.

rsmith updated this revision to Diff 159153.Aug 3 2018, 6:20 PM
rsmith marked an inline comment as done.
rsmith added inline comments.
lib/CodeGen/CGExpr.cpp
521

The OptimizationLevel check is dead anyway (we don't emit lifetime markers unless we're optimizing or in use-after-scope mode). Removed.

rjmccall accepted this revision.Aug 3 2018, 6:23 PM

LGTM.

This revision is now accepted and ready to land.Aug 3 2018, 6:23 PM
Closed by commit rC338945: Avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end… (authored by rsmith). · Explain WhyAug 3 2018, 6:25 PM
This revision was automatically updated to reflect the committed changes.
rsmith marked an inline comment as done.

Revision Contents

PathSize
lib/
CodeGen/
47 lines
test/
CodeGenCXX/
164 lines
42 lines

Diff 159154

lib/CodeGen/CGExpr.cpp

Show First 20 Lines • Show All 492 Lines • ▼ Show 20 Lines if (auto *Var = dyn_cast<llvm::GlobalVariable>(
// initialized it. // initialized it.
if (!Var->hasInitializer()) { if (!Var->hasInitializer()) {
Var->setInitializer(CGM.EmitNullConstant(E->getType())); Var->setInitializer(CGM.EmitNullConstant(E->getType()));
EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true); EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true);
} }
} else { } else {
switch (M->getStorageDuration()) { switch (M->getStorageDuration()) {
case SD_Automatic: case SD_Automatic:
case SD_FullExpression:
if (auto *Size = EmitLifetimeStart( if (auto *Size = EmitLifetimeStart(
CGM.getDataLayout().getTypeAllocSize(Alloca.getElementType()), CGM.getDataLayout().getTypeAllocSize(Alloca.getElementType()),
Alloca.getPointer())) { Alloca.getPointer())) {
if (M->getStorageDuration() == SD_Automatic)
pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalEHLifetimeMarker, pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalEHLifetimeMarker,
Alloca, Size); Alloca, Size);
else }
break;
case SD_FullExpression: {
if (!ShouldEmitLifetimeMarkers)
break;
// Avoid creating a conditional cleanup just to hold an llvm.lifetime.end
// marker. Instead, start the lifetime of a conditional temporary earlier
// so that it's unconditional. Don't do this in ASan's use-after-scope
// mode so that it gets the more precise lifetime marks. If the type has
// a non-trivial destructor, we'll have a cleanup block for it anyway,
// so this typically doesn't help; skip it in that case.
ConditionalEvaluation *OldConditional = nullptr;
CGBuilderTy::InsertPoint OldIP;
if (isInConditionalBranch() && !E->getType().isDestructedType() &&
rjmccallUnsubmitted
Done
ReplyInline Actions

Why only when optimization is enabled? This seems like a nice improvement regardless.

Also, please mention why this is disabled for destructed types in the comment, not just the commit message.

rjmccall: Why only when optimization is enabled? This seems like a nice improvement regardless. Also…
rsmithAuthorUnsubmitted
Not Done
ReplyInline Actions

The OptimizationLevel check is dead anyway (we don't emit lifetime markers unless we're optimizing or in use-after-scope mode). Removed.

rsmith: The `OptimizationLevel` check is dead anyway (we don't emit lifetime markers unless we're…
!CGM.getCodeGenOpts().SanitizeAddressUseAfterScope) {
OldConditional = OutermostConditional;
OutermostConditional = nullptr;
OldIP = Builder.saveIP();
llvm::BasicBlock *Block = OldConditional->getStartingBlock();
Builder.restoreIP(CGBuilderTy::InsertPoint(
Block, llvm::BasicBlock::iterator(Block->back())));
}
if (auto *Size = EmitLifetimeStart(
CGM.getDataLayout().getTypeAllocSize(Alloca.getElementType()),
Alloca.getPointer())) {
pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker, Alloca, pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker, Alloca,
Size); Size);
} }
if (OldConditional) {
OutermostConditional = OldConditional;
Builder.restoreIP(OldIP);
}
break; break;
}
default: default:
break; break;
} }
EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true); EmitAnyExprToMem(E, Object, Qualifiers(), /*IsInit*/true);
} }
pushTemporaryCleanup(*this, M, E, Object); pushTemporaryCleanup(*this, M, E, Object);
// Perform derived-to-base casts and/or field accesses, to get from the // Perform derived-to-base casts and/or field accesses, to get from the
▲ Show 20 Lines • Show All 4,359 Lines • Show Last 20 Lines

test/CodeGenCXX/conditional-temporaries.cpp

// REQUIRES: amdgpu-registered-target // REQUIRES: amdgpu-registered-target
// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -O3 | FileCheck %s // RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -O2 -disable-llvm-passes | FileCheck %s --check-prefixes=CHECK,CHECK-NOOPT
// RUN: %clang_cc1 -emit-llvm %s -o - -triple=amdgcn-amd-amdhsa -O3 | FileCheck %s // RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -O2 | FileCheck %s --check-prefixes=CHECK,CHECK-OPT
// RUN: %clang_cc1 -emit-llvm %s -o - -triple=amdgcn-amd-amdhsa -O2 | FileCheck %s --check-prefixes=CHECK,CHECK-OPT
namespace { namespace {
static int ctorcalls; static int ctorcalls;
static int dtorcalls; static int dtorcalls;
struct A { struct A {
A() : i(0) { ctorcalls++; } A() : i(0) { ctorcalls++; }
Show All 21 Lines Checker() {
f1(false); f1(false);
} }
}; };
Checker c; Checker c;
} }
// CHECK-LABEL: define i32 @_Z12getCtorCallsv() // CHECK-OPT-LABEL: define i32 @_Z12getCtorCallsv()
int getCtorCalls() { int getCtorCalls() {
// CHECK: ret i32 5 // CHECK-OPT: ret i32 5
return ctorcalls; return ctorcalls;
} }
// CHECK-LABEL: define i32 @_Z12getDtorCallsv() // CHECK-OPT-LABEL: define i32 @_Z12getDtorCallsv()
int getDtorCalls() { int getDtorCalls() {
// CHECK: ret i32 5 // CHECK-OPT: ret i32 5
return dtorcalls; return dtorcalls;
} }
// CHECK-LABEL: define zeroext i1 @_Z7successv() // CHECK-OPT-LABEL: define zeroext i1 @_Z7successv()
bool success() { bool success() {
// CHECK: ret i1 true // CHECK-OPT: ret i1 true
return ctorcalls == dtorcalls; return ctorcalls == dtorcalls;
} }
struct X { ~X(); int f(); };
int g(int, int, int);
// CHECK-LABEL: @_Z16lifetime_nontriv
int lifetime_nontriv(bool cond) {
// CHECK-NOOPT: store i1 false,
// CHECK-NOOPT: store i1 false,
// CHECK-NOOPT: store i1 false,
// CHECK-NOOPT: store i1 false,
// CHECK-NOOPT: store i1 false,
// CHECK-NOOPT: store i1 false,
// CHECK-NOOPT: br i1
//
// CHECK-NOOPT: call void @llvm.lifetime.start
// CHECK-NOOPT: store i1 true,
// CHECK-NOOPT: store i1 true,
// CHECK-NOOPT: call i32 @_ZN1X1fEv(
// CHECK-NOOPT: call void @llvm.lifetime.start
// CHECK-NOOPT: store i1 true,
// CHECK-NOOPT: store i1 true,
// CHECK-NOOPT: call i32 @_ZN1X1fEv(
// CHECK-NOOPT: call void @llvm.lifetime.start
// CHECK-NOOPT: store i1 true,
// CHECK-NOOPT: store i1 true,
// CHECK-NOOPT: call i32 @_ZN1X1fEv(
// CHECK-NOOPT: call i32 @_Z1giii(
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: call i32 @_Z1giii(i32 1, i32 2, i32 3)
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: load i1,
// CHECK-NOOPT: br i1
// CHECK-NOOPT: call void @_ZN1XD1Ev(
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: load i1,
// CHECK-NOOPT: br i1
// CHECK-NOOPT: call void @llvm.lifetime.end
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: load i1,
// CHECK-NOOPT: br i1
// CHECK-NOOPT: call void @_ZN1XD1Ev(
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: load i1,
// CHECK-NOOPT: br i1
// CHECK-NOOPT: call void @llvm.lifetime.end
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: load i1,
// CHECK-NOOPT: br i1
// CHECK-NOOPT: call void @_ZN1XD1Ev(
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: load i1,
// CHECK-NOOPT: br i1
// CHECK-NOOPT: call void @llvm.lifetime.end
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: ret
// CHECK-OPT: br i1
//
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call i32 @_ZN1X1fEv(
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call i32 @_ZN1X1fEv(
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call i32 @_ZN1X1fEv(
// CHECK-OPT: call i32 @_Z1giii(
// CHECK-OPT: call void @_ZN1XD1Ev(
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: call void @_ZN1XD1Ev(
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: call void @_ZN1XD1Ev(
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: br label
return cond ? g(X().f(), X().f(), X().f()) : g(1, 2, 3);
}
struct Y { int f(); };
int g(int, int, int);
// CHECK-LABEL: @_Z13lifetime_triv
int lifetime_triv(bool cond) {
// CHECK-NOOPT: call void @llvm.lifetime.start
// CHECK-NOOPT: call void @llvm.lifetime.start
// CHECK-NOOPT: call void @llvm.lifetime.start
// CHECK-NOOPT: br i1
//
// CHECK-NOOPT: call i32 @_ZN1Y1fEv(
// CHECK-NOOPT: call i32 @_ZN1Y1fEv(
// CHECK-NOOPT: call i32 @_ZN1Y1fEv(
// CHECK-NOOPT: call i32 @_Z1giii(
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: call i32 @_Z1giii(i32 1, i32 2, i32 3)
// CHECK-NOOPT: br label
//
// CHECK-NOOPT: call void @llvm.lifetime.end
// CHECK-NOOPT-NOT: br
// CHECK-NOOPT: call void @llvm.lifetime.end
// CHECK-NOOPT-NOT: br
// CHECK-NOOPT: call void @llvm.lifetime.end
//
// CHECK-NOOPT: ret
// FIXME: LLVM isn't smart enough to remove the lifetime markers from the
// g(1, 2, 3) path here.
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: br i1
//
// CHECK-OPT: call i32 @_ZN1Y1fEv(
// CHECK-OPT: call i32 @_ZN1Y1fEv(
// CHECK-OPT: call i32 @_ZN1Y1fEv(
// CHECK-OPT: call i32 @_Z1giii(
// CHECK-OPT: br label
//
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: call void @llvm.lifetime.end
return cond ? g(Y().f(), Y().f(), Y().f()) : g(1, 2, 3);
}
struct Z { ~Z() {} int f(); };
int g(int, int, int);
// CHECK-LABEL: @_Z22lifetime_nontriv_empty
int lifetime_nontriv_empty(bool cond) {
// CHECK-OPT: br i1
//
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call i32 @_ZN1Z1fEv(
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call i32 @_ZN1Z1fEv(
// CHECK-OPT: call void @llvm.lifetime.start
// CHECK-OPT: call i32 @_ZN1Z1fEv(
// CHECK-OPT: call i32 @_Z1giii(
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: call void @llvm.lifetime.end
// CHECK-OPT: br label
return cond ? g(Z().f(), Z().f(), Z().f()) : g(1, 2, 3);
}

test/CodeGenCXX/lifetime-asan.cpp

// RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -o - -fno-exceptions -O0 %s | FileCheck %s -check-prefixes=CHECK,CHECK-O0 --implicit-check-not=llvm.lifetime
// RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -o - -fno-exceptions -O0 \
// RUN: -fsanitize=address -fsanitize-address-use-after-scope %s | \
// RUN: FileCheck %s -check-prefixes=CHECK,CHECK-ASAN-USE-AFTER-SCOPE
extern int bar(char *A, int n);
struct X { X(); ~X(); int *p; };
struct Y { Y(); int *p; };
extern "C" void a(), b(), c(), d();
// CHECK-LABEL: @_Z3foo
void foo(int n) {
// CHECK: call void @a()
a();
// CHECK: call void @b()
// CHECK-ASAN-USE-AFTER-SCOPE: store i1 false
// CHECK-ASAN-USE-AFTER-SCOPE: store i1 false
// CHECK: br i1
//
// CHECK-ASAN-USE-AFTER-SCOPE: @llvm.lifetime.start
// CHECK-ASAN-USE-AFTER-SCOPE: store i1 true
// CHECK: call void @_ZN1XC
// CHECK: br label
//
// CHECK-ASAN-USE-AFTER-SCOPE: @llvm.lifetime.start
// CHECK-ASAN-USE-AFTER-SCOPE: store i1 true
// CHECK: call void @_ZN1YC
// CHECK: br label
//
// CHECK: call void @c()
// CHECK-ASAN-USE-AFTER-SCOPE: br i1
// CHECK-ASAN-USE-AFTER-SCOPE: @llvm.lifetime.end
// CHECK-ASAN-USE-AFTER-SCOPE: br i1
// CHECK-ASAN-USE-AFTER-SCOPE: @llvm.lifetime.end
b(), (n ? X().p : Y().p), c();
// CHECK: call void @d()
d();
}