[CodeGen] Try to not call a dtor after lifetime.end

If CodeGenFunction::EmitCall is:
- asked to emit a call with an indirectly returned value,
- given an invalid return value slot, and
- told the return value of the function it's calling is unused

then it'll make its own temporary, and add lifetime markers so that the
temporary's lifetime ends immediately after the call.

The early lifetime.end becomes problematic when we need to run a
destructor on the result of the function.

Instead of unconditionally saying that results of all calls are used
here (which would be correct, but would also cause us to never emit
lifetime markers for these temporaries), we just build our own temporary
to pass in when a dtor has to be run.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@327192 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/CodeGen/CGExprAgg.cpp b/lib/CodeGen/CGExprAgg.cpp
index 394336ee1225bf7cb5c23d2231bf8721dc18f6a1..5613f8d9700deced91c6e8856d0e520f9a2d65cc 100644 (file)
--- a/lib/CodeGen/CGExprAgg.cpp
+++ b/lib/CodeGen/CGExprAgg.cpp
@@ -37,23 +37,6 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
   AggValueSlot Dest;
   bool IsResultUnused;
 
-  /// We want to use 'dest' as the return slot except under two
-  /// conditions:
-  ///   - The destination slot requires garbage collection, so we
-  ///     need to use the GC API.
-  ///   - The destination slot is potentially aliased.
-  bool shouldUseDestForReturnSlot() const {
-    return !(Dest.requiresGCollection() || Dest.isPotentiallyAliased());
-  }
-
-  ReturnValueSlot getReturnValueSlot() const {
-    if (!shouldUseDestForReturnSlot())
-      return ReturnValueSlot();
-
-    return ReturnValueSlot(Dest.getAddress(), Dest.isVolatile(),
-                           IsResultUnused);
-  }
-
   AggValueSlot EnsureSlot(QualType T) {
     if (!Dest.isIgnored()) return Dest;
     return CGF.CreateAggTemp(T, "agg.tmp.ensured");
@@ -63,6 +46,15 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
     Dest = CGF.CreateAggTemp(T, "agg.tmp.ensured");
   }
 
+  // Calls `Fn` with a valid return value slot, potentially creating a temporary
+  // to do so. If a temporary is created, an appropriate copy into `Dest` will
+  // be emitted.
+  //
+  // The given function should take a ReturnValueSlot, and return an RValue that
+  // points to said slot.
+  void withReturnValueSlot(const Expr *E,
+                           llvm::function_ref<RValue(ReturnValueSlot)> Fn);
+
 public:
   AggExprEmitter(CodeGenFunction &cgf, AggValueSlot Dest, bool IsResultUnused)
     : CGF(cgf), Builder(CGF.Builder), Dest(Dest),
@@ -242,34 +234,44 @@ bool AggExprEmitter::TypeRequiresGCollection(QualType T) {
   return Record->hasObjectMember();
 }
 
-/// \brief Perform the final move to DestPtr if for some reason
-/// getReturnValueSlot() didn't use it directly.
-///
-/// The idea is that you do something like this:
-///   RValue Result = EmitSomething(..., getReturnValueSlot());
-///   EmitMoveFromReturnSlot(E, Result);
-///
-/// If nothing interferes, this will cause the result to be emitted
-/// directly into the return value slot.  Otherwise, a final move
-/// will be performed.
-void AggExprEmitter::EmitMoveFromReturnSlot(const Expr *E, RValue src) {
-  // Push destructor if the result is ignored and the type is a C struct that
-  // is non-trivial to destroy.
-  QualType Ty = E->getType();
-  if (Dest.isIgnored() &&
-      Ty.isDestructedType() == QualType::DK_nontrivial_c_struct)
-    CGF.pushDestroy(Ty.isDestructedType(), src.getAggregateAddress(), Ty);
-
-  if (shouldUseDestForReturnSlot()) {
-    // Logically, Dest.getAddr() should equal Src.getAggregateAddr().
-    // The possibility of undef rvalues complicates that a lot,
-    // though, so we can't really assert.
-    return;
+void AggExprEmitter::withReturnValueSlot(
+    const Expr *E, llvm::function_ref<RValue(ReturnValueSlot)> EmitCall) {
+  QualType RetTy = E->getType();
+  bool RequiresDestruction =
+      Dest.isIgnored() &&
+      RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct;
+
+  // If it makes no observable difference, save a memcpy + temporary.
+  //
+  // We need to always provide our own temporary if destruction is required.
+  // Otherwise, EmitCall will emit its own, notice that it's "unused", and end
+  // its lifetime before we have the chance to emit a proper destructor call.
+  bool UseTemp = Dest.isPotentiallyAliased() || Dest.requiresGCollection() ||
+                 (RequiresDestruction && !Dest.getAddress().isValid());
+
+  Address RetAddr = Address::invalid();
+  if (!UseTemp) {
+    RetAddr = Dest.getAddress();
+  } else {
+    RetAddr = CGF.CreateMemTemp(RetTy);
+    uint64_t Size =
+        CGF.CGM.getDataLayout().getTypeAllocSize(CGF.ConvertTypeForMem(RetTy));
+    if (llvm::Value *LifetimeSizePtr =
+            CGF.EmitLifetimeStart(Size, RetAddr.getPointer()))
+      CGF.pushFullExprCleanup<CodeGenFunction::CallLifetimeEnd>(
+          NormalEHLifetimeMarker, RetAddr, LifetimeSizePtr);
   }
 
-  // Otherwise, copy from there to the destination.
-  assert(Dest.getPointer() != src.getAggregatePointer());
-  EmitFinalDestCopy(E->getType(), src);
+  RValue Src =
+      EmitCall(ReturnValueSlot(RetAddr, Dest.isVolatile(), IsResultUnused));
+
+  if (RequiresDestruction)
+    CGF.pushDestroy(RetTy.isDestructedType(), Src.getAggregateAddress(), RetTy);
+
+  if (UseTemp) {
+    assert(Dest.getPointer() != Src.getAggregatePointer());
+    EmitFinalDestCopy(E->getType(), Src);
+  }
 }
 
 /// EmitFinalDestCopy - Perform the final copy to DestPtr, if desired.
@@ -828,13 +830,15 @@ void AggExprEmitter::VisitCallExpr(const CallExpr *E) {
     return;
   }
 
-  RValue RV = CGF.EmitCallExpr(E, getReturnValueSlot());
-  EmitMoveFromReturnSlot(E, RV);
+  withReturnValueSlot(E, [&](ReturnValueSlot Slot) {
+    return CGF.EmitCallExpr(E, Slot);
+  });
 }
 
 void AggExprEmitter::VisitObjCMessageExpr(ObjCMessageExpr *E) {
-  RValue RV = CGF.EmitObjCMessageExpr(E, getReturnValueSlot());
-  EmitMoveFromReturnSlot(E, RV);
+  withReturnValueSlot(E, [&](ReturnValueSlot Slot) {
+    return CGF.EmitObjCMessageExpr(E, Slot);
+  });
 }
 
 void AggExprEmitter::VisitBinComma(const BinaryOperator *E) {

diff --git a/test/CodeGenObjC/arc.m b/test/CodeGenObjC/arc.m
index 7871369413eef96e826b281e9b7714593a77810e..3243952a36b3aff7773278c12be3a741445f4c5d 100644 (file)
--- a/test/CodeGenObjC/arc.m
+++ b/test/CodeGenObjC/arc.m
@@ -1526,6 +1526,37 @@ void test70(id i) {
   };
 }
 
+// Be sure that we emit lifetime intrinsics only after dtors
+struct AggDtor {
+  char cs[40];
+  id x;
+};
+
+struct AggDtor getAggDtor(void);
+
+// CHECK-LABEL: define void @test71
+void test71(void) {
+  // FIXME: It would be nice if the __destructor_8_s40 for the first call (and
+  // the following lifetime.end) came before the second call.
+  //
+  // CHECK: %[[T:[^ ]+]] = bitcast %struct.AggDtor* %[[TMP1:[^ ]+]] to i8*
+  // CHECK: call void @llvm.lifetime.start.p0i8({{[^,]+}}, i8* %[[T]])
+  // CHECK: call void @getAggDtor(%struct.AggDtor* sret %[[TMP1]])
+  // CHECK: %[[T:[^ ]+]] = bitcast %struct.AggDtor* %[[TMP2:[^ ]+]] to i8*
+  // CHECK: call void @llvm.lifetime.start.p0i8({{[^,]+}}, i8* %[[T]])
+  // CHECK: call void @getAggDtor(%struct.AggDtor* sret %[[TMP2]])
+  // CHECK: %[[T:[^ ]+]] = bitcast %struct.AggDtor* %[[TMP2]] to i8**
+  // CHECK: call void @__destructor_8_s40(i8** %[[T]])
+  // CHECK: %[[T:[^ ]+]] = bitcast %struct.AggDtor* %[[TMP2:[^ ]+]] to i8*
+  // CHECK: call void @llvm.lifetime.end.p0i8({{[^,]+}}, i8* %[[T]])
+  // CHECK: %[[T:[^ ]+]] = bitcast %struct.AggDtor* %[[TMP1]] to i8**
+  // CHECK: call void @__destructor_8_s40(i8** %[[T]])
+  // CHECK: %[[T:[^ ]+]] = bitcast %struct.AggDtor* %[[TMP1:[^ ]+]] to i8*
+  // CHECK: call void @llvm.lifetime.end.p0i8({{[^,]+}}, i8* %[[T]])
+  getAggDtor();
+  getAggDtor();
+}
+
 // ARC-ALIEN: attributes [[NLB]] = { nonlazybind }
 // ARC-NATIVE: attributes [[NLB]] = { nonlazybind }
 // CHECK: attributes [[NUW]] = { nounwind }