ahh created this revision.
ahh added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.
[expr.delete] is pretty crystal clear that it's OK to invoke a
deallocation-function on a nullptr delete-expression:
"If the value of the operand of the delete-expression is a null
pointer value, it is unspecified whether a deallocation function will
be called as described above."
Even so, we currently check against nullptr unconditionally. This is
wasteful for anything with a trivial destructor; deleting nullptr is
very rare so it's not worth saving the call to ::operator delete, and
this is a useless branch (and waste of code size) in the common case.
Condition emission of the branch on us needing to actually look
through the pointer for a vtable, size cookie, or nontrivial
destructor. (In principle a nontrivial destructor with no side
effects that didn't touch the object would also be safe to run
unconditionally, but I don't know how to test we have one of those and
who in the world would write one?)
On an important and very large (~500 MiB) Google search binary, this
saves approximately 32 KiB of text. Okay, it's not impressive in a
relative sense, but it's still the right thing to do.
A note on optimization: we still successfully elide delete-expressions of
(literal) nullptr. Where before they were stuck behind a never-taken branch,
now they reduce (effectively) to calls to __builtin_operator_delete(nullptr),
which EarlyCSE is capable of optimizing out. So this has no cost in the
already well-optimized case.
Repository:
rC Clang
https://reviews.llvm.org/D43430
Files:
lib/CodeGen/CGCXXABI.h
lib/CodeGen/CGExprCXX.cpp
test/CodeGenCXX/cxx2a-destroying-delete.cpp
test/CodeGenCXX/delete-two-arg.cpp
test/CodeGenCXX/delete.cpp
Index: test/CodeGenCXX/delete.cpp
===================================================================
--- test/CodeGenCXX/delete.cpp
+++ test/CodeGenCXX/delete.cpp
@@ -9,7 +9,12 @@
};
// POD types.
+// CHECK-LABEL: define void @_Z2t3P1S
void t3(S *s) {
+ // CHECK-NOT: icmp eq {{.*}}, null
+ // CHECK-NOT: br i1
+ // CHECK: call void @_ZdlPv
+
delete s;
}
@@ -99,6 +104,8 @@
namespace test3 {
void f(int a[10][20]) {
+ // CHECK-NOT: icmp eq {{.*}}, null
+ // CHECK-NOT: br i1
// CHECK: call void @_ZdaPv(i8*
delete a;
}
@@ -113,6 +120,8 @@
// CHECK-LABEL: define void @_ZN5test421global_delete_virtualEPNS_1XE
void global_delete_virtual(X *xp) {
+ // CHECK: icmp eq {{.*}}, null
+ // CHECK: br i1
// Load the offset-to-top from the vtable and apply it.
// This has to be done first because the dtor can mess it up.
// CHECK: [[T0:%.*]] = bitcast [[X:%.*]]* [[XP:%.*]] to i64**
Index: test/CodeGenCXX/delete-two-arg.cpp
===================================================================
--- test/CodeGenCXX/delete-two-arg.cpp
+++ test/CodeGenCXX/delete-two-arg.cpp
@@ -9,8 +9,8 @@
// CHECK-LABEL: define void @_ZN5test11aEPNS_1AE(
void a(A *x) {
// CHECK: load
- // CHECK-NEXT: icmp eq {{.*}}, null
- // CHECK-NEXT: br i1
+ // CHECK-NOT: icmp eq {{.*}}, null
+ // CHECK-NOT: br i1
// CHECK: call void @_ZN5test11AdlEPvj(i8* %{{.*}}, i32 4)
delete x;
}
Index: test/CodeGenCXX/cxx2a-destroying-delete.cpp
===================================================================
--- test/CodeGenCXX/cxx2a-destroying-delete.cpp
+++ test/CodeGenCXX/cxx2a-destroying-delete.cpp
@@ -15,9 +15,8 @@
void delete_A(A *a) { delete a; }
// CHECK-LABEL: define {{.*}}delete_A
// CHECK: %[[a:.*]] = load
-// CHECK: icmp eq %{{.*}} %[[a]], null
-// CHECK: br i1
-//
+// CHECK-NOT: icmp eq %{{.*}} %[[a]], null
+// CHECK-NOT: br i1
// Ensure that we call the destroying delete and not the destructor.
// CHECK-NOT: call
// CHECK-ITANIUM: call void @_ZN1AdlEPS_St19destroying_delete_t(%{{.*}}* %[[a]])
@@ -60,8 +59,8 @@
// CHECK: %[[castbase:.*]] = bitcast {{.*}} %[[base]]
//
// CHECK: %[[a:.*]] = phi {{.*}} %[[castbase]]
-// CHECK: icmp eq %{{.*}} %[[a]], null
-// CHECK: br i1
+// CHECK-NOT: icmp eq %{{.*}} %[[a]], null
+// CHECK-NOT: br i1
//
// CHECK-NOT: call
// CHECK-ITANIUM: call void @_ZN1AdlEPS_St19destroying_delete_t(%{{.*}}* %[[a]])
Index: lib/CodeGen/CGExprCXX.cpp
===================================================================
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -1971,26 +1971,58 @@
CGF.PopCleanupBlock();
}
-void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) {
- const Expr *Arg = E->getArgument();
- Address Ptr = EmitPointerWithAlignment(Arg);
+// Will we read through the deleted pointer? If so,
+// we must first check it is not null.
+static bool DeleteMightAccessObject(CodeGenFunction &CGF,
+ const CXXDeleteExpr *E,
+ QualType DeleteTy) {
- // Null check the pointer.
- llvm::BasicBlock *DeleteNotNull = createBasicBlock("delete.notnull");
- llvm::BasicBlock *DeleteEnd = createBasicBlock("delete.end");
+ if (E->getOperatorDelete()->isDestroyingOperatorDelete()) {
+ // It is safe to call destroying operator delete with nullptr arguments
+ // ([expr.delete] tells us it is unspecified whether a deallocation
+ // function is called) but a virtual destructor must be resolved
+ // to find the right function, which we can't do on nullptr.
+ auto *Dtor = DeleteTy->getAsCXXRecordDecl()->getDestructor();
+ return Dtor && Dtor->isVirtual();
+ }
- llvm::Value *IsNull = Builder.CreateIsNull(Ptr.getPointer(), "isnull");
+ if (E->isArrayForm()) {
+ return CGF.CGM.getCXXABI().requiresArrayCookie(E, DeleteTy);
+ }
- Builder.CreateCondBr(IsNull, DeleteEnd, DeleteNotNull);
- EmitBlock(DeleteNotNull);
+ // Otherwise, we should avoid invoking any nontrivial destructor on
+ // a null object.
+ return DeleteTy.isDestructedType();
+}
+void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) {
+ const Expr *Arg = E->getArgument();
+ Address Ptr = EmitPointerWithAlignment(Arg);
QualType DeleteTy = E->getDestroyedType();
+ // Null check the pointer, unless the destructor is trivial. In that case,
+ // all we'll be doing is passing Ptr to ::operator delete(), which is
+ // well formed for nullptr arguments (and allowed by [expr.delete.7]
+ // The overwhelming majority of deletes are of non-nullptr, so there's
+ // no efficiency gain to be had by skipping the very rare exceptions, and
+ // it bleeds code size (and unneeded branches.)
+ llvm::BasicBlock *DeleteEnd = nullptr;
+ if (DeleteMightAccessObject(*this, E, DeleteTy)) {
+ llvm::BasicBlock *DeleteNotNull = createBasicBlock("delete.notnull");
+ DeleteEnd = createBasicBlock("delete.end");
+
+ llvm::Value *IsNull = Builder.CreateIsNull(Ptr.getPointer(), "isnull");
+
+ Builder.CreateCondBr(IsNull, DeleteEnd, DeleteNotNull);
+ EmitBlock(DeleteNotNull);
+ }
+
+
// A destroying operator delete overrides the entire operation of the
// delete expression.
if (E->getOperatorDelete()->isDestroyingOperatorDelete()) {
EmitDestroyingObjectDelete(*this, E, Ptr, DeleteTy);
- EmitBlock(DeleteEnd);
+ if (DeleteEnd) EmitBlock(DeleteEnd);
return;
}
@@ -2018,14 +2050,14 @@
}
assert(ConvertTypeForMem(DeleteTy) == Ptr.getElementType());
-
+
if (E->isArrayForm()) {
EmitArrayDelete(*this, E, Ptr, DeleteTy);
} else {
EmitObjectDelete(*this, E, Ptr, DeleteTy);
}
- EmitBlock(DeleteEnd);
+ if (DeleteEnd) EmitBlock(DeleteEnd);
}
static bool isGLValueFromPointerDeref(const Expr *E) {
Index: lib/CodeGen/CGCXXABI.h
===================================================================
--- lib/CodeGen/CGCXXABI.h
+++ lib/CodeGen/CGCXXABI.h
@@ -80,8 +80,7 @@
ASTContext &getContext() const { return CGM.getContext(); }
- virtual bool requiresArrayCookie(const CXXDeleteExpr *E, QualType eltType);
- virtual bool requiresArrayCookie(const CXXNewExpr *E);
+
/// Determine whether there's something special about the rules of
/// the ABI tell us that 'this' is a complete object within the
@@ -461,6 +460,9 @@
/**************************** Array cookies ******************************/
+ virtual bool requiresArrayCookie(const CXXDeleteExpr *E, QualType eltType);
+ virtual bool requiresArrayCookie(const CXXNewExpr *E);
+
/// Returns the extra size required in order to store the array
/// cookie for the given new-expression. May return 0 to indicate that no
/// array cookie is required.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
