[llvm] f39e12a - PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.
Richard Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 5 17:13:55 PDT 2020
Author: Richard Smith
Date: 2020-06-05T17:13:43-07:00
New Revision: f39e12a06b6018db195848ca1f7bd01bf0240fac
URL: https://github.com/llvm/llvm-project/commit/f39e12a06b6018db195848ca1f7bd01bf0240fac
DIFF: https://github.com/llvm/llvm-project/commit/f39e12a06b6018db195848ca1f7bd01bf0240fac.diff
LOG: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.
Summary:
This transformation is correct for a builtin call to 'free(p)', but not
for 'operator delete(p)'. There is no guarantee that a user replacement
'operator delete' has no effect when called on a null pointer.
However, the principle behind the transformation *is* correct, and can
be applied more broadly: a 'delete p' expression is permitted to
unconditionally call 'operator delete(p)'. So do that in Clang under
-Oz where possible. We do this whether or not 'p' has trivial
destruction, since the destruction might turn out to be trivial after
inlining, and even for a class-specific (but non-virtual,
non-destroying, non-array) 'operator delete'.
Reviewers: davide, dnsampaio, rjmccall
Reviewed By: dnsampaio
Subscribers: hiraditya, cfe-commits, llvm-commits
Tags: #clang, #llvm
Differential Revision: https://reviews.llvm.org/D79378
Added:
Modified:
clang/lib/CodeGen/CGExprCXX.cpp
clang/test/CodeGenCXX/delete.cpp
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
llvm/test/Transforms/InstCombine/malloc-free-delete.ll
Removed:
################################################################################
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index 5bca92470f6f..dfae2bc5e47a 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -1874,10 +1874,13 @@ static void EmitDestroyingObjectDelete(CodeGenFunction &CGF,
}
/// Emit the code for deleting a single object.
-static void EmitObjectDelete(CodeGenFunction &CGF,
+/// \return \c true if we started emitting UnconditionalDeleteBlock, \c false
+/// if not.
+static bool EmitObjectDelete(CodeGenFunction &CGF,
const CXXDeleteExpr *DE,
Address Ptr,
- QualType ElementType) {
+ QualType ElementType,
+ llvm::BasicBlock *UnconditionalDeleteBlock) {
// C++11 [expr.delete]p3:
// If the static type of the object to be deleted is
diff erent from its
// dynamic type, the static type shall be a base class of the dynamic type
@@ -1924,7 +1927,7 @@ static void EmitObjectDelete(CodeGenFunction &CGF,
if (UseVirtualCall) {
CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,
Dtor);
- return;
+ return false;
}
}
}
@@ -1959,7 +1962,15 @@ static void EmitObjectDelete(CodeGenFunction &CGF,
}
}
+ // When optimizing for size, call 'operator delete' unconditionally.
+ if (CGF.CGM.getCodeGenOpts().OptimizeSize > 1) {
+ CGF.EmitBlock(UnconditionalDeleteBlock);
+ CGF.PopCleanupBlock();
+ return true;
+ }
+
CGF.PopCleanupBlock();
+ return false;
}
namespace {
@@ -2036,6 +2047,12 @@ void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) {
Address Ptr = EmitPointerWithAlignment(Arg);
// Null check the pointer.
+ //
+ // We could avoid this null check if we can determine that the object
+ // destruction is trivial and doesn't require an array cookie; we can
+ // unconditionally perform the operator delete call in that case. For now, we
+ // assume that deleted pointers are null rarely enough that it's better to
+ // keep the branch. This might be worth revisiting for a -O0 code size win.
llvm::BasicBlock *DeleteNotNull = createBasicBlock("delete.notnull");
llvm::BasicBlock *DeleteEnd = createBasicBlock("delete.end");
@@ -2081,11 +2098,11 @@ void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) {
if (E->isArrayForm()) {
EmitArrayDelete(*this, E, Ptr, DeleteTy);
+ EmitBlock(DeleteEnd);
} else {
- EmitObjectDelete(*this, E, Ptr, DeleteTy);
+ if (!EmitObjectDelete(*this, E, Ptr, DeleteTy, DeleteEnd))
+ EmitBlock(DeleteEnd);
}
-
- EmitBlock(DeleteEnd);
}
static bool isGLValueFromPointerDeref(const Expr *E) {
diff --git a/clang/test/CodeGenCXX/delete.cpp b/clang/test/CodeGenCXX/delete.cpp
index ff448f808dcb..5172b5e01af5 100644
--- a/clang/test/CodeGenCXX/delete.cpp
+++ b/clang/test/CodeGenCXX/delete.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,CHECK-NOSIZE
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -emit-llvm -o - -Oz -disable-llvm-passes | FileCheck %s --check-prefixes=CHECK,CHECK-SIZE
void t1(int *a) {
delete a;
@@ -9,7 +10,19 @@ struct S {
};
// POD types.
+
+// CHECK-LABEL: define void @_Z2t3P1S
void t3(S *s) {
+ // CHECK: icmp {{.*}} null
+ // CHECK: br i1
+
+ // CHECK: bitcast
+ // CHECK-NEXT: call void @_ZdlPv
+
+ // Check the delete is inside the 'if !null' check unless we're optimizing
+ // for size. FIXME: We could omit the branch entirely in this case.
+ // CHECK-NOSIZE-NEXT: br
+ // CHECK-SIZE-NEXT: ret
delete s;
}
@@ -22,7 +35,9 @@ struct T {
// CHECK-LABEL: define void @_Z2t4P1T
void t4(T *t) {
// CHECK: call void @_ZN1TD1Ev
- // CHECK-NEXT: bitcast
+ // CHECK-NOSIZE-NEXT: bitcast
+ // CHECK-SIZE-NEXT: br
+ // CHECK-SIZE: bitcast
// CHECK-NEXT: call void @_ZdlPv
delete t;
}
@@ -49,7 +64,9 @@ namespace test0 {
// CHECK-LABEL: define void @_ZN5test04testEPNS_1AE(
void test(A *a) {
// CHECK: call void @_ZN5test01AD1Ev
- // CHECK-NEXT: bitcast
+ // CHECK-NOSIZE-NEXT: bitcast
+ // CHECK-SIZE-NEXT: br
+ // CHECK-SIZE: bitcast
// CHECK-NEXT: call void @_ZN5test01AdlEPv
delete a;
}
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 3b405ee6f86e..9a62793a7116 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2670,9 +2670,16 @@ Instruction *InstCombiner::visitFree(CallInst &FI) {
// if (foo) free(foo);
// into
// free(foo);
- if (MinimizeSize)
- if (Instruction *I = tryToMoveFreeBeforeNullTest(FI, DL))
- return I;
+ //
+ // Note that we can only do this for 'free' and not for any flavor of
+ // 'operator delete'; there is no 'operator delete' symbol for which we are
+ // permitted to invent a call, even if we're passing in a null pointer.
+ if (MinimizeSize) {
+ LibFunc Func;
+ if (TLI.getLibFunc(FI, Func) && TLI.has(Func) && Func == LibFunc_free)
+ if (Instruction *I = tryToMoveFreeBeforeNullTest(FI, DL))
+ return I;
+ }
return nullptr;
}
diff --git a/llvm/test/Transforms/InstCombine/malloc-free-delete.ll b/llvm/test/Transforms/InstCombine/malloc-free-delete.ll
index 2f4447b65118..1b3e682905d6 100644
--- a/llvm/test/Transforms/InstCombine/malloc-free-delete.ll
+++ b/llvm/test/Transforms/InstCombine/malloc-free-delete.ll
@@ -140,6 +140,31 @@ if.end: ; preds = %entry, %if.then
ret void
}
+; Same optimization with even a builtin 'operator delete' would be
+; incorrect in general.
+; 'if (p) delete p;' cannot result in a call to 'operator delete(0)'.
+define void @test6a(i8* %foo) minsize {
+; CHECK-LABEL: @test6a(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[TOBOOL:%.*]] = icmp eq i8* [[FOO:%.*]], null
+; CHECK-NEXT: br i1 [[TOBOOL]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: tail call void @_ZdlPv(i8* [[FOO]])
+; CHECK-NEXT: br label [[IF_END]]
+; CHECK: if.end:
+; CHECK-NEXT: ret void
+entry:
+ %tobool = icmp eq i8* %foo, null
+ br i1 %tobool, label %if.end, label %if.then
+
+if.then: ; preds = %entry
+ tail call void @_ZdlPv(i8* %foo) builtin
+ br label %if.end
+
+if.end: ; preds = %entry, %if.then
+ ret void
+}
+
declare i8* @_ZnwmRKSt9nothrow_t(i64, i8*) nobuiltin
declare void @_ZdlPvRKSt9nothrow_t(i8*, i8*) nobuiltin
declare i32 @__gxx_personality_v0(...)
More information about the llvm-commits
mailing list