PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

zygoloid · zygoloid · commit f39e12a06b60 · 2020-06-05T17:13:43.000-07:00
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
diff --git 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 different 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
@@ -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
@@ -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
@@ -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(...)
